From de6a0857991ad1898f0c2806788101426c545619 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Tue, 6 May 2025 16:37:48 -0700 Subject: [PATCH 01/27] add matrix to stress --- packages/dds/matrix/package.json | 12 + packages/dds/matrix/src/test/fuzz.ts | 223 ++++++++++++++++++ packages/dds/matrix/src/test/index.ts | 6 + .../dds/matrix/src/test/matrix.fuzz.spec.ts | 216 +---------------- .../local-server-stress-tests/package.json | 4 +- .../src/ddsModels.ts | 3 + pnpm-lock.yaml | 21 +- 7 files changed, 262 insertions(+), 223 deletions(-) create mode 100644 packages/dds/matrix/src/test/fuzz.ts create mode 100644 packages/dds/matrix/src/test/index.ts diff --git a/packages/dds/matrix/package.json b/packages/dds/matrix/package.json index a203a5cf9c6c..2a11c61ad8b6 100644 --- a/packages/dds/matrix/package.json +++ b/packages/dds/matrix/package.json @@ -42,6 +42,18 @@ "types": "./dist/index.d.ts", "default": "./dist/index.js" } + }, + "./internal/test": { + "allow-ff-test-exports": { + "import": { + "types": "./lib/test/index.d.ts", + "default": "./lib/test/index.js" + }, + "require": { + "types": "./dist/test/index.d.ts", + "default": "./dist/test/index.js" + } + } } }, "main": "lib/index.js", diff --git a/packages/dds/matrix/src/test/fuzz.ts b/packages/dds/matrix/src/test/fuzz.ts new file mode 100644 index 000000000000..f9b53818cfd4 --- /dev/null +++ b/packages/dds/matrix/src/test/fuzz.ts @@ -0,0 +1,223 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +import { strict as assert } from "node:assert"; + +import { + AsyncGenerator, + Generator, + combineReducers, + createWeightedGenerator, + takeAsync, +} from "@fluid-private/stochastic-test-utils"; +import { DDSFuzzTestState, type DDSFuzzModel } from "@fluid-private/test-dds-utils"; +import type { IFluidHandle } from "@fluidframework/core-interfaces"; +import { isObject } from "@fluidframework/core-utils/internal"; +import type { Serializable } from "@fluidframework/datastore-definitions/internal"; +import { isFluidHandle } from "@fluidframework/runtime-utils/internal"; + +import { MatrixItem } from "../ops.js"; +import { SharedMatrixFactory, SharedMatrix } from "../runtime.js"; + +/** + * Supported cell values used within the fuzz model. + */ +type Value = string | number | undefined | Serializable; + +interface RangeSpec { + start: number; + count: number; +} + +interface InsertRows extends RangeSpec { + type: "insertRows"; +} + +interface InsertColumns extends RangeSpec { + type: "insertCols"; +} + +interface RemoveRows extends RangeSpec { + type: "removeRows"; +} + +interface RemoveColumns extends RangeSpec { + type: "removeCols"; +} + +interface SetCell { + type: "set"; + row: number; + col: number; + value: MatrixItem; +} + +export type Operation = InsertRows | InsertColumns | RemoveRows | RemoveColumns | SetCell; + +// This type gets used a lot as the state object of the suite; shorthand it here. +type State = DDSFuzzTestState; + +async function assertMatricesAreEquivalent( + a: SharedMatrix, + b: SharedMatrix, +): Promise { + assert.equal( + a.colCount, + b.colCount, + `${a.id} and ${b.id} have different number of columns.`, + ); + assert.equal(a.rowCount, b.rowCount, `${a.id} and ${b.id} have different number of rows.`); + for (let row = 0; row < a.rowCount; row++) { + for (let col = 0; col < a.colCount; col++) { + const aVal = a.getCell(row, col); + const bVal = b.getCell(row, col); + if (isObject(aVal) === true) { + assert( + isObject(bVal), + `${a.id} and ${b.id} differ at (${row}, ${col}): a is an object, b is not`, + ); + const aHandle = isFluidHandle(aVal) ? await aVal.get() : aVal; + const bHandle = isFluidHandle(bVal) ? await bVal.get() : bVal; + assert.deepEqual( + aHandle, + bHandle, + `${a.id} and ${b.id} differ at (${row}, ${col}): ${JSON.stringify( + aHandle, + )} vs ${JSON.stringify(bHandle)}`, + ); + } else { + assert.equal( + aVal, + bVal, + `${a.id} and ${b.id} differ at (${row}, ${col}): ${aVal} vs ${bVal}`, + ); + } + } + } +} + +const reducer = combineReducers({ + insertRows: ({ client }, { start, count }) => { + client.channel.insertRows(start, count); + }, + insertCols: ({ client }, { start, count }) => { + client.channel.insertCols(start, count); + }, + removeRows: ({ client }, { start, count }) => { + client.channel.removeRows(start, count); + }, + removeCols: ({ client }, { start, count }) => { + client.channel.removeCols(start, count); + }, + set: ({ client }, { row, col, value }) => { + client.channel.setCell(row, col, value); + }, +}); + +interface GeneratorOptions { + insertRowWeight: number; + insertColWeight: number; + removeRowWeight: number; + removeColWeight: number; + setWeight: number; +} + +const defaultOptions: GeneratorOptions = { + insertRowWeight: 1, + insertColWeight: 1, + removeRowWeight: 1, + removeColWeight: 1, + setWeight: 20, +}; + +function makeGenerator( + optionsParam?: Partial, +): AsyncGenerator { + const { setWeight, insertColWeight, insertRowWeight, removeRowWeight, removeColWeight } = { + ...defaultOptions, + ...optionsParam, + }; + + const maxDimensionSizeChange = 3; + + const insertRows: Generator = ({ random, client }) => ({ + type: "insertRows", + start: random.integer(0, client.channel.rowCount), + count: random.integer(1, maxDimensionSizeChange), + }); + + const removeRows: Generator = ({ random, client }) => { + const start = random.integer(0, client.channel.rowCount - 1); + const count = random.integer( + 1, + Math.min(maxDimensionSizeChange, client.channel.rowCount - start), + ); + return { + type: "removeRows", + start, + count, + }; + }; + + const removeCols: Generator = ({ random, client }) => { + const start = random.integer(0, client.channel.colCount - 1); + const count = random.integer( + 1, + Math.min(maxDimensionSizeChange, client.channel.colCount - start), + ); + return { + type: "removeCols", + start, + count, + }; + }; + + const insertCols: Generator = ({ random, client }) => ({ + type: "insertCols", + start: random.integer(0, client.channel.colCount), + count: random.integer(1, maxDimensionSizeChange), + }); + + const setKey: Generator = ({ random, client }) => ({ + type: "set", + row: random.integer(0, client.channel.rowCount - 1), + col: random.integer(0, client.channel.colCount - 1), + value: random.pick([ + (): number => random.integer(1, 50), + (): string => random.string(random.integer(1, 2)), + (): IFluidHandle => random.handle(), + ])(), + }); + + const syncGenerator = createWeightedGenerator([ + [ + setKey, + setWeight, + (state): boolean => + state.client.channel.rowCount > 0 && state.client.channel.colCount > 0, + ], + [insertRows, insertRowWeight], + [insertCols, insertColWeight], + [removeRows, removeRowWeight, (state): boolean => state.client.channel.rowCount > 0], + [removeCols, removeColWeight, (state): boolean => state.client.channel.colCount > 0], + ]); + + return async (state) => syncGenerator(state); +} + +export const baseSharedMatrixModel: Omit< + DDSFuzzModel, + "workloadName" +> = { + factory: SharedMatrix.getFactory(), + generatorFactory: () => takeAsync(50, makeGenerator()), + reducer: (state, operation) => reducer(state, operation), + validateConsistency: async (a, b) => assertMatricesAreEquivalent(a.channel, b.channel), + minimizationTransforms: ["count", "start", "row", "col"].map((p) => (op) => { + if (p in op && typeof op[p] === "number" && op[p] > 0) { + op[p]--; + } + }), +}; diff --git a/packages/dds/matrix/src/test/index.ts b/packages/dds/matrix/src/test/index.ts new file mode 100644 index 000000000000..33a296e8caaf --- /dev/null +++ b/packages/dds/matrix/src/test/index.ts @@ -0,0 +1,6 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +export { baseSharedMatrixModel } from "./fuzz.js"; diff --git a/packages/dds/matrix/src/test/matrix.fuzz.spec.ts b/packages/dds/matrix/src/test/matrix.fuzz.spec.ts index e724ae6b4997..a8b29795e92f 100644 --- a/packages/dds/matrix/src/test/matrix.fuzz.spec.ts +++ b/packages/dds/matrix/src/test/matrix.fuzz.spec.ts @@ -3,218 +3,19 @@ * Licensed under the MIT License. */ -import { strict as assert } from "node:assert"; import * as path from "node:path"; -import { - AsyncGenerator, - Generator, - combineReducers, - createWeightedGenerator, - takeAsync, -} from "@fluid-private/stochastic-test-utils"; import { DDSFuzzModel, DDSFuzzSuiteOptions, - DDSFuzzTestState, createDDSFuzzSuite, } from "@fluid-private/test-dds-utils"; -import type { IFluidHandle } from "@fluidframework/core-interfaces"; -import { isObject } from "@fluidframework/core-utils/internal"; -import type { Serializable } from "@fluidframework/datastore-definitions/internal"; import { FlushMode } from "@fluidframework/runtime-definitions/internal"; -import { isFluidHandle } from "@fluidframework/runtime-utils/internal"; -import { MatrixItem } from "../ops.js"; -import { SharedMatrixFactory, SharedMatrix } from "../runtime.js"; +import { SharedMatrixFactory } from "../runtime.js"; import { _dirname } from "./dirname.cjs"; - -/** - * Supported cell values used within the fuzz model. - */ -type Value = string | number | undefined | Serializable; - -interface RangeSpec { - start: number; - count: number; -} - -interface InsertRows extends RangeSpec { - type: "insertRows"; -} - -interface InsertColumns extends RangeSpec { - type: "insertCols"; -} - -interface RemoveRows extends RangeSpec { - type: "removeRows"; -} - -interface RemoveColumns extends RangeSpec { - type: "removeCols"; -} - -interface SetCell { - type: "set"; - row: number; - col: number; - value: MatrixItem; -} - -type Operation = InsertRows | InsertColumns | RemoveRows | RemoveColumns | SetCell; - -// This type gets used a lot as the state object of the suite; shorthand it here. -type State = DDSFuzzTestState; - -async function assertMatricesAreEquivalent( - a: SharedMatrix, - b: SharedMatrix, -): Promise { - assert.equal( - a.colCount, - b.colCount, - `${a.id} and ${b.id} have different number of columns.`, - ); - assert.equal(a.rowCount, b.rowCount, `${a.id} and ${b.id} have different number of rows.`); - for (let row = 0; row < a.rowCount; row++) { - for (let col = 0; col < a.colCount; col++) { - const aVal = a.getCell(row, col); - const bVal = b.getCell(row, col); - if (isObject(aVal) === true) { - assert( - isObject(bVal), - `${a.id} and ${b.id} differ at (${row}, ${col}): a is an object, b is not`, - ); - const aHandle = isFluidHandle(aVal) ? await aVal.get() : aVal; - const bHandle = isFluidHandle(bVal) ? await bVal.get() : bVal; - assert.deepEqual( - aHandle, - bHandle, - `${a.id} and ${b.id} differ at (${row}, ${col}): ${JSON.stringify( - aHandle, - )} vs ${JSON.stringify(bHandle)}`, - ); - } else { - assert.equal( - aVal, - bVal, - `${a.id} and ${b.id} differ at (${row}, ${col}): ${aVal} vs ${bVal}`, - ); - } - } - } -} - -const reducer = combineReducers({ - insertRows: ({ client }, { start, count }) => { - client.channel.insertRows(start, count); - }, - insertCols: ({ client }, { start, count }) => { - client.channel.insertCols(start, count); - }, - removeRows: ({ client }, { start, count }) => { - client.channel.removeRows(start, count); - }, - removeCols: ({ client }, { start, count }) => { - client.channel.removeCols(start, count); - }, - set: ({ client }, { row, col, value }) => { - client.channel.setCell(row, col, value); - }, -}); - -interface GeneratorOptions { - insertRowWeight: number; - insertColWeight: number; - removeRowWeight: number; - removeColWeight: number; - setWeight: number; -} - -const defaultOptions: GeneratorOptions = { - insertRowWeight: 1, - insertColWeight: 1, - removeRowWeight: 1, - removeColWeight: 1, - setWeight: 20, -}; - -function makeGenerator( - optionsParam?: Partial, -): AsyncGenerator { - const { setWeight, insertColWeight, insertRowWeight, removeRowWeight, removeColWeight } = { - ...defaultOptions, - ...optionsParam, - }; - - const maxDimensionSizeChange = 3; - - const insertRows: Generator = ({ random, client }) => ({ - type: "insertRows", - start: random.integer(0, client.channel.rowCount), - count: random.integer(1, maxDimensionSizeChange), - }); - - const removeRows: Generator = ({ random, client }) => { - const start = random.integer(0, client.channel.rowCount - 1); - const count = random.integer( - 1, - Math.min(maxDimensionSizeChange, client.channel.rowCount - start), - ); - return { - type: "removeRows", - start, - count, - }; - }; - - const removeCols: Generator = ({ random, client }) => { - const start = random.integer(0, client.channel.colCount - 1); - const count = random.integer( - 1, - Math.min(maxDimensionSizeChange, client.channel.colCount - start), - ); - return { - type: "removeCols", - start, - count, - }; - }; - - const insertCols: Generator = ({ random, client }) => ({ - type: "insertCols", - start: random.integer(0, client.channel.colCount), - count: random.integer(1, maxDimensionSizeChange), - }); - - const setKey: Generator = ({ random, client }) => ({ - type: "set", - row: random.integer(0, client.channel.rowCount - 1), - col: random.integer(0, client.channel.colCount - 1), - value: random.pick([ - (): number => random.integer(1, 50), - (): string => random.string(random.integer(1, 2)), - (): IFluidHandle => random.handle(), - ])(), - }); - - const syncGenerator = createWeightedGenerator([ - [ - setKey, - setWeight, - (state): boolean => - state.client.channel.rowCount > 0 && state.client.channel.colCount > 0, - ], - [insertRows, insertRowWeight], - [insertCols, insertColWeight], - [removeRows, removeRowWeight, (state): boolean => state.client.channel.rowCount > 0], - [removeCols, removeColWeight, (state): boolean => state.client.channel.colCount > 0], - ]); - - return async (state) => syncGenerator(state); -} +import { baseSharedMatrixModel, type Operation } from "./fuzz.js"; describe("Matrix fuzz tests", function () { /** @@ -229,17 +30,6 @@ describe("Matrix fuzz tests", function () { * underlying harness (which affects which seeds are the slow ones). */ this.timeout(30_000); - const model: Omit, "workloadName"> = { - factory: SharedMatrix.getFactory(), - generatorFactory: () => takeAsync(50, makeGenerator()), - reducer: (state, operation) => reducer(state, operation), - validateConsistency: async (a, b) => assertMatricesAreEquivalent(a.channel, b.channel), - minimizationTransforms: ["count", "start", "row", "col"].map((p) => (op) => { - if (p in op && typeof op[p] === "number" && op[p] > 0) { - op[p]--; - } - }), - }; const baseOptions: Partial = { defaultTestCount: 100, @@ -253,7 +43,7 @@ describe("Matrix fuzz tests", function () { }; const nameModel = (workloadName: string): DDSFuzzModel => ({ - ...model, + ...baseSharedMatrixModel, workloadName, }); diff --git a/packages/test/local-server-stress-tests/package.json b/packages/test/local-server-stress-tests/package.json index 6d7ce15a2ace..de890fac693f 100644 --- a/packages/test/local-server-stress-tests/package.json +++ b/packages/test/local-server-stress-tests/package.json @@ -76,6 +76,7 @@ "@fluidframework/id-compressor": "workspace:~", "@fluidframework/local-driver": "workspace:~", "@fluidframework/map": "workspace:~", + "@fluidframework/matrix": "workspace:~", "@fluidframework/runtime-definitions": "workspace:~", "@fluidframework/runtime-utils": "workspace:~", "@fluidframework/sequence": "workspace:~", @@ -105,7 +106,8 @@ "^tsc", "^api-extractor:commonjs", "@fluidframework/sequence#build:test", - "@fluidframework/map#build:test" + "@fluidframework/map#build:test", + "@fluidframework/matrix#build:test" ] } }, diff --git a/packages/test/local-server-stress-tests/src/ddsModels.ts b/packages/test/local-server-stress-tests/src/ddsModels.ts index 7b2df9db91a1..5f3ca9f908d1 100644 --- a/packages/test/local-server-stress-tests/src/ddsModels.ts +++ b/packages/test/local-server-stress-tests/src/ddsModels.ts @@ -8,6 +8,8 @@ import { DDSFuzzModel, DDSFuzzTestState } from "@fluid-private/test-dds-utils"; import type { IChannelFactory } from "@fluidframework/datastore-definitions/internal"; // eslint-disable-next-line import/no-internal-modules import { baseMapModel, baseDirModel } from "@fluidframework/map/internal/test"; +// eslint-disable-next-line import/no-internal-modules +import { baseSharedMatrixModel } from "@fluidframework/matrix/internal/test"; import { baseSharedStringModel, baseIntervalModel, @@ -65,4 +67,5 @@ export const ddsModelMap = generateSubModelMap( baseDirModel, baseSharedStringModel, baseIntervalModel, + baseSharedMatrixModel, ); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 91663f8e98f3..50daba3e75ed 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -13541,6 +13541,9 @@ importers: '@fluidframework/map': specifier: workspace:~ version: link:../../dds/map + '@fluidframework/matrix': + specifier: workspace:~ + version: link:../../dds/matrix '@fluidframework/runtime-definitions': specifier: workspace:~ version: link:../../runtime/runtime-definitions @@ -30298,9 +30301,9 @@ snapshots: '@typescript-eslint/parser': 6.7.5(eslint@8.55.0)(typescript@5.4.5) eslint-config-biome: 1.9.4 eslint-config-prettier: 9.0.0(eslint@8.55.0) - eslint-import-resolver-typescript: 3.6.3(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-plugin-i@2.29.1(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint@8.55.0))(eslint@8.55.0) + eslint-import-resolver-typescript: 3.6.3(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-plugin-i@2.29.1)(eslint@8.55.0) eslint-plugin-eslint-comments: 3.2.0(eslint@8.55.0) - eslint-plugin-import: eslint-plugin-i@2.29.1(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-import-resolver-typescript@3.6.3(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-plugin-i@2.29.1(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint@8.55.0))(eslint@8.55.0))(eslint@8.55.0) + eslint-plugin-import: eslint-plugin-i@2.29.1(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-import-resolver-typescript@3.6.3)(eslint@8.55.0) eslint-plugin-jsdoc: 46.8.2(eslint@8.55.0) eslint-plugin-promise: 6.1.1(eslint@8.55.0) eslint-plugin-react: 7.33.2(eslint@8.55.0) @@ -36143,33 +36146,33 @@ snapshots: transitivePeerDependencies: - supports-color - eslint-import-resolver-typescript@3.6.3(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-plugin-i@2.29.1(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint@8.55.0))(eslint@8.55.0): + eslint-import-resolver-typescript@3.6.3(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-plugin-i@2.29.1)(eslint@8.55.0): dependencies: '@nolyfill/is-core-module': 1.0.39 debug: 4.4.0(supports-color@8.1.1) enhanced-resolve: 5.17.1 eslint: 8.55.0 - eslint-module-utils: 2.12.0(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-import-resolver-node@0.3.9)(eslint-import-resolver-typescript@3.6.3(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-plugin-i@2.29.1(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint@8.55.0))(eslint@8.55.0))(eslint@8.55.0) + eslint-module-utils: 2.12.0(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-import-resolver-node@0.3.9)(eslint-import-resolver-typescript@3.6.3)(eslint@8.55.0) fast-glob: 3.3.3 get-tsconfig: 4.10.0 is-bun-module: 1.3.0 is-glob: 4.0.3 optionalDependencies: - eslint-plugin-import: eslint-plugin-i@2.29.1(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-import-resolver-typescript@3.6.3(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-plugin-i@2.29.1(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint@8.55.0))(eslint@8.55.0))(eslint@8.55.0) + eslint-plugin-import: eslint-plugin-i@2.29.1(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-import-resolver-typescript@3.6.3)(eslint@8.55.0) transitivePeerDependencies: - '@typescript-eslint/parser' - eslint-import-resolver-node - eslint-import-resolver-webpack - supports-color - eslint-module-utils@2.12.0(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-import-resolver-node@0.3.9)(eslint-import-resolver-typescript@3.6.3(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-plugin-i@2.29.1(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint@8.55.0))(eslint@8.55.0))(eslint@8.55.0): + eslint-module-utils@2.12.0(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-import-resolver-node@0.3.9)(eslint-import-resolver-typescript@3.6.3)(eslint@8.55.0): dependencies: debug: 3.2.7 optionalDependencies: '@typescript-eslint/parser': 6.7.5(eslint@8.55.0)(typescript@5.4.5) eslint: 8.55.0 eslint-import-resolver-node: 0.3.9 - eslint-import-resolver-typescript: 3.6.3(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-plugin-i@2.29.1(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint@8.55.0))(eslint@8.55.0) + eslint-import-resolver-typescript: 3.6.3(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-plugin-i@2.29.1)(eslint@8.55.0) transitivePeerDependencies: - supports-color @@ -36183,13 +36186,13 @@ snapshots: eslint: 8.55.0 ignore: 5.3.2 - eslint-plugin-i@2.29.1(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-import-resolver-typescript@3.6.3(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-plugin-i@2.29.1(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint@8.55.0))(eslint@8.55.0))(eslint@8.55.0): + eslint-plugin-i@2.29.1(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-import-resolver-typescript@3.6.3)(eslint@8.55.0): dependencies: debug: 4.4.0(supports-color@8.1.1) doctrine: 3.0.0 eslint: 8.55.0 eslint-import-resolver-node: 0.3.9 - eslint-module-utils: 2.12.0(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-import-resolver-node@0.3.9)(eslint-import-resolver-typescript@3.6.3(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-plugin-i@2.29.1(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint@8.55.0))(eslint@8.55.0))(eslint@8.55.0) + eslint-module-utils: 2.12.0(@typescript-eslint/parser@6.7.5(eslint@8.55.0)(typescript@5.4.5))(eslint-import-resolver-node@0.3.9)(eslint-import-resolver-typescript@3.6.3)(eslint@8.55.0) get-tsconfig: 4.10.0 is-glob: 4.0.3 minimatch: 3.1.2 From c1ab401613140a50a5b918820843ee6107aad8f0 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Wed, 14 May 2025 09:44:36 -0700 Subject: [PATCH 02/27] Add FFW to matrix fuzz --- packages/dds/matrix/src/test/fuzz.ts | 235 ++++++++++++++++++ .../dds/matrix/src/test/matrix.fuzz.spec.ts | 216 +--------------- 2 files changed, 238 insertions(+), 213 deletions(-) create mode 100644 packages/dds/matrix/src/test/fuzz.ts diff --git a/packages/dds/matrix/src/test/fuzz.ts b/packages/dds/matrix/src/test/fuzz.ts new file mode 100644 index 000000000000..ea08559fb8c5 --- /dev/null +++ b/packages/dds/matrix/src/test/fuzz.ts @@ -0,0 +1,235 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +import { strict as assert } from "node:assert"; + +import { + AsyncGenerator, + Generator, + combineReducers, + createWeightedGenerator, + takeAsync, +} from "@fluid-private/stochastic-test-utils"; +import { DDSFuzzTestState, type DDSFuzzModel } from "@fluid-private/test-dds-utils"; +import type { IFluidHandle } from "@fluidframework/core-interfaces"; +import type { Serializable } from "@fluidframework/datastore-definitions/internal"; +import { isFluidHandle, toFluidHandleInternal } from "@fluidframework/runtime-utils/internal"; + +import { MatrixItem } from "../ops.js"; +import { SharedMatrixFactory, SharedMatrix } from "../runtime.js"; + +/** + * Supported cell values used within the fuzz model. + */ +type Value = string | number | undefined | Serializable; + +interface RangeSpec { + start: number; + count: number; +} + +interface InsertRows extends RangeSpec { + type: "insertRows"; +} + +interface InsertColumns extends RangeSpec { + type: "insertCols"; +} + +interface RemoveRows extends RangeSpec { + type: "removeRows"; +} + +interface RemoveColumns extends RangeSpec { + type: "removeCols"; +} + +interface SetCell { + type: "set"; + row: number; + col: number; + value: MatrixItem; +} + +interface SwitchSetCellPolicy { + type: "switchSetCellPolicy"; +} + +export type Operation = + | InsertRows + | InsertColumns + | RemoveRows + | RemoveColumns + | SetCell + | SwitchSetCellPolicy; + +// This type gets used a lot as the state object of the suite; shorthand it here. +type State = DDSFuzzTestState; + +async function assertMatricesAreEquivalent( + a: SharedMatrix, + b: SharedMatrix, +): Promise { + assert.equal( + a.colCount, + b.colCount, + `${a.id} and ${b.id} have different number of columns.`, + ); + assert.equal(a.rowCount, b.rowCount, `${a.id} and ${b.id} have different number of rows.`); + for (let row = 0; row < a.rowCount; row++) { + for (let col = 0; col < a.colCount; col++) { + const aVal = a.getCell(row, col); + const bVal = b.getCell(row, col); + const aHandle = isFluidHandle(aVal) ? toFluidHandleInternal(aVal).absolutePath : aVal; + const bHandle = isFluidHandle(bVal) ? toFluidHandleInternal(bVal).absolutePath : bVal; + assert.deepEqual( + aHandle, + bHandle, + `${a.id} and ${b.id} differ at (${row}, ${col}): ${JSON.stringify( + aHandle, + )} vs ${JSON.stringify(bHandle)}`, + ); + } + } +} + +const reducer = combineReducers({ + insertRows: ({ client }, { start, count }) => { + client.channel.insertRows(start, count); + }, + insertCols: ({ client }, { start, count }) => { + client.channel.insertCols(start, count); + }, + removeRows: ({ client }, { start, count }) => { + client.channel.removeRows(start, count); + }, + removeCols: ({ client }, { start, count }) => { + client.channel.removeCols(start, count); + }, + set: ({ client }, { row, col, value }) => { + client.channel.setCell(row, col, value); + }, + switchSetCellPolicy: ({ client }) => { + client.channel.switchSetCellPolicy(); + }, +}); + +type GeneratorOptions = Record<`${Operation["type"]}Weight`, number>; + +const defaultOptions: GeneratorOptions = { + insertRowsWeight: 10, + insertColsWeight: 10, + removeRowsWeight: 10, + removeColsWeight: 10, + setWeight: 200, + switchSetCellPolicyWeight: 1, +}; + +function makeGenerator( + optionsParam?: Partial, +): AsyncGenerator { + const { + setWeight, + insertColsWeight, + insertRowsWeight, + removeRowsWeight, + removeColsWeight, + switchSetCellPolicyWeight, + } = { + ...defaultOptions, + ...optionsParam, + }; + + const maxDimensionSizeChange = 3; + + const insertRows: Generator = ({ random, client }) => ({ + type: "insertRows", + start: random.integer(0, client.channel.rowCount), + count: random.integer(1, maxDimensionSizeChange), + }); + + const removeRows: Generator = ({ random, client }) => { + const start = random.integer(0, client.channel.rowCount - 1); + const count = random.integer( + 1, + Math.min(maxDimensionSizeChange, client.channel.rowCount - start), + ); + return { + type: "removeRows", + start, + count, + }; + }; + + const removeCols: Generator = ({ random, client }) => { + const start = random.integer(0, client.channel.colCount - 1); + const count = random.integer( + 1, + Math.min(maxDimensionSizeChange, client.channel.colCount - start), + ); + return { + type: "removeCols", + start, + count, + }; + }; + + const insertCols: Generator = ({ random, client }) => ({ + type: "insertCols", + start: random.integer(0, client.channel.colCount), + count: random.integer(1, maxDimensionSizeChange), + }); + + const setKey: Generator = ({ random, client }) => ({ + type: "set", + row: random.integer(0, client.channel.rowCount - 1), + col: random.integer(0, client.channel.colCount - 1), + value: random.pick([ + (): number => random.integer(1, 50), + (): string => random.string(random.integer(1, 2)), + (): IFluidHandle => random.handle(), + ])(), + }); + + const switchSetCellPolicy: Generator = () => ({ + type: "switchSetCellPolicy", + }); + + const syncGenerator = createWeightedGenerator([ + [ + setKey, + setWeight, + (state): boolean => + state.client.channel.rowCount > 0 && state.client.channel.colCount > 0, + ], + [insertRows, insertRowsWeight], + [insertCols, insertColsWeight], + [removeRows, removeRowsWeight, (state): boolean => state.client.channel.rowCount > 0], + [removeCols, removeColsWeight, (state): boolean => state.client.channel.colCount > 0], + [ + switchSetCellPolicy, + switchSetCellPolicyWeight, + (state): boolean => + state.client.channel.isSetCellConflictResolutionPolicyFWW() === false, + ], + ]); + + return async (state) => syncGenerator(state); +} + +export const baseSharedMatrixModel: Omit< + DDSFuzzModel, + "workloadName" +> = { + factory: SharedMatrix.getFactory(), + generatorFactory: () => takeAsync(50, makeGenerator()), + reducer: (state, operation) => reducer(state, operation), + validateConsistency: async (a, b) => assertMatricesAreEquivalent(a.channel, b.channel), + minimizationTransforms: ["count", "start", "row", "col"].map((p) => (op) => { + if (p in op && typeof op[p] === "number" && op[p] > 0) { + op[p]--; + } + }), +}; diff --git a/packages/dds/matrix/src/test/matrix.fuzz.spec.ts b/packages/dds/matrix/src/test/matrix.fuzz.spec.ts index e724ae6b4997..a8b29795e92f 100644 --- a/packages/dds/matrix/src/test/matrix.fuzz.spec.ts +++ b/packages/dds/matrix/src/test/matrix.fuzz.spec.ts @@ -3,218 +3,19 @@ * Licensed under the MIT License. */ -import { strict as assert } from "node:assert"; import * as path from "node:path"; -import { - AsyncGenerator, - Generator, - combineReducers, - createWeightedGenerator, - takeAsync, -} from "@fluid-private/stochastic-test-utils"; import { DDSFuzzModel, DDSFuzzSuiteOptions, - DDSFuzzTestState, createDDSFuzzSuite, } from "@fluid-private/test-dds-utils"; -import type { IFluidHandle } from "@fluidframework/core-interfaces"; -import { isObject } from "@fluidframework/core-utils/internal"; -import type { Serializable } from "@fluidframework/datastore-definitions/internal"; import { FlushMode } from "@fluidframework/runtime-definitions/internal"; -import { isFluidHandle } from "@fluidframework/runtime-utils/internal"; -import { MatrixItem } from "../ops.js"; -import { SharedMatrixFactory, SharedMatrix } from "../runtime.js"; +import { SharedMatrixFactory } from "../runtime.js"; import { _dirname } from "./dirname.cjs"; - -/** - * Supported cell values used within the fuzz model. - */ -type Value = string | number | undefined | Serializable; - -interface RangeSpec { - start: number; - count: number; -} - -interface InsertRows extends RangeSpec { - type: "insertRows"; -} - -interface InsertColumns extends RangeSpec { - type: "insertCols"; -} - -interface RemoveRows extends RangeSpec { - type: "removeRows"; -} - -interface RemoveColumns extends RangeSpec { - type: "removeCols"; -} - -interface SetCell { - type: "set"; - row: number; - col: number; - value: MatrixItem; -} - -type Operation = InsertRows | InsertColumns | RemoveRows | RemoveColumns | SetCell; - -// This type gets used a lot as the state object of the suite; shorthand it here. -type State = DDSFuzzTestState; - -async function assertMatricesAreEquivalent( - a: SharedMatrix, - b: SharedMatrix, -): Promise { - assert.equal( - a.colCount, - b.colCount, - `${a.id} and ${b.id} have different number of columns.`, - ); - assert.equal(a.rowCount, b.rowCount, `${a.id} and ${b.id} have different number of rows.`); - for (let row = 0; row < a.rowCount; row++) { - for (let col = 0; col < a.colCount; col++) { - const aVal = a.getCell(row, col); - const bVal = b.getCell(row, col); - if (isObject(aVal) === true) { - assert( - isObject(bVal), - `${a.id} and ${b.id} differ at (${row}, ${col}): a is an object, b is not`, - ); - const aHandle = isFluidHandle(aVal) ? await aVal.get() : aVal; - const bHandle = isFluidHandle(bVal) ? await bVal.get() : bVal; - assert.deepEqual( - aHandle, - bHandle, - `${a.id} and ${b.id} differ at (${row}, ${col}): ${JSON.stringify( - aHandle, - )} vs ${JSON.stringify(bHandle)}`, - ); - } else { - assert.equal( - aVal, - bVal, - `${a.id} and ${b.id} differ at (${row}, ${col}): ${aVal} vs ${bVal}`, - ); - } - } - } -} - -const reducer = combineReducers({ - insertRows: ({ client }, { start, count }) => { - client.channel.insertRows(start, count); - }, - insertCols: ({ client }, { start, count }) => { - client.channel.insertCols(start, count); - }, - removeRows: ({ client }, { start, count }) => { - client.channel.removeRows(start, count); - }, - removeCols: ({ client }, { start, count }) => { - client.channel.removeCols(start, count); - }, - set: ({ client }, { row, col, value }) => { - client.channel.setCell(row, col, value); - }, -}); - -interface GeneratorOptions { - insertRowWeight: number; - insertColWeight: number; - removeRowWeight: number; - removeColWeight: number; - setWeight: number; -} - -const defaultOptions: GeneratorOptions = { - insertRowWeight: 1, - insertColWeight: 1, - removeRowWeight: 1, - removeColWeight: 1, - setWeight: 20, -}; - -function makeGenerator( - optionsParam?: Partial, -): AsyncGenerator { - const { setWeight, insertColWeight, insertRowWeight, removeRowWeight, removeColWeight } = { - ...defaultOptions, - ...optionsParam, - }; - - const maxDimensionSizeChange = 3; - - const insertRows: Generator = ({ random, client }) => ({ - type: "insertRows", - start: random.integer(0, client.channel.rowCount), - count: random.integer(1, maxDimensionSizeChange), - }); - - const removeRows: Generator = ({ random, client }) => { - const start = random.integer(0, client.channel.rowCount - 1); - const count = random.integer( - 1, - Math.min(maxDimensionSizeChange, client.channel.rowCount - start), - ); - return { - type: "removeRows", - start, - count, - }; - }; - - const removeCols: Generator = ({ random, client }) => { - const start = random.integer(0, client.channel.colCount - 1); - const count = random.integer( - 1, - Math.min(maxDimensionSizeChange, client.channel.colCount - start), - ); - return { - type: "removeCols", - start, - count, - }; - }; - - const insertCols: Generator = ({ random, client }) => ({ - type: "insertCols", - start: random.integer(0, client.channel.colCount), - count: random.integer(1, maxDimensionSizeChange), - }); - - const setKey: Generator = ({ random, client }) => ({ - type: "set", - row: random.integer(0, client.channel.rowCount - 1), - col: random.integer(0, client.channel.colCount - 1), - value: random.pick([ - (): number => random.integer(1, 50), - (): string => random.string(random.integer(1, 2)), - (): IFluidHandle => random.handle(), - ])(), - }); - - const syncGenerator = createWeightedGenerator([ - [ - setKey, - setWeight, - (state): boolean => - state.client.channel.rowCount > 0 && state.client.channel.colCount > 0, - ], - [insertRows, insertRowWeight], - [insertCols, insertColWeight], - [removeRows, removeRowWeight, (state): boolean => state.client.channel.rowCount > 0], - [removeCols, removeColWeight, (state): boolean => state.client.channel.colCount > 0], - ]); - - return async (state) => syncGenerator(state); -} +import { baseSharedMatrixModel, type Operation } from "./fuzz.js"; describe("Matrix fuzz tests", function () { /** @@ -229,17 +30,6 @@ describe("Matrix fuzz tests", function () { * underlying harness (which affects which seeds are the slow ones). */ this.timeout(30_000); - const model: Omit, "workloadName"> = { - factory: SharedMatrix.getFactory(), - generatorFactory: () => takeAsync(50, makeGenerator()), - reducer: (state, operation) => reducer(state, operation), - validateConsistency: async (a, b) => assertMatricesAreEquivalent(a.channel, b.channel), - minimizationTransforms: ["count", "start", "row", "col"].map((p) => (op) => { - if (p in op && typeof op[p] === "number" && op[p] > 0) { - op[p]--; - } - }), - }; const baseOptions: Partial = { defaultTestCount: 100, @@ -253,7 +43,7 @@ describe("Matrix fuzz tests", function () { }; const nameModel = (workloadName: string): DDSFuzzModel => ({ - ...model, + ...baseSharedMatrixModel, workloadName, }); From 011d7336bc72e83c5de8666e3cee46b8ba5723dd Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Wed, 14 May 2025 09:46:13 -0700 Subject: [PATCH 03/27] rever lss --- packages/test/local-server-stress-tests/CHANGELOG.md | 4 ++++ packages/test/local-server-stress-tests/package.json | 6 ++---- .../test/local-server-stress-tests/src/ddsModels.ts | 3 --- .../local-server-stress-tests/src/stressDataObject.ts | 5 +++-- .../src/test/localServerStress.spec.ts | 11 +++++++---- .../test/localServerStressOrderSequentially.spec.ts | 11 ++++------- 6 files changed, 20 insertions(+), 20 deletions(-) diff --git a/packages/test/local-server-stress-tests/CHANGELOG.md b/packages/test/local-server-stress-tests/CHANGELOG.md index bc7aca235a8a..5a051a346ccb 100644 --- a/packages/test/local-server-stress-tests/CHANGELOG.md +++ b/packages/test/local-server-stress-tests/CHANGELOG.md @@ -1,5 +1,9 @@ # @fluid-internal/local-server-stress-tests +## 2.40.0 + +Dependency updates only. + ## 2.33.0 Dependency updates only. diff --git a/packages/test/local-server-stress-tests/package.json b/packages/test/local-server-stress-tests/package.json index de890fac693f..e751b034f8ce 100644 --- a/packages/test/local-server-stress-tests/package.json +++ b/packages/test/local-server-stress-tests/package.json @@ -1,6 +1,6 @@ { "name": "@fluid-internal/local-server-stress-tests", - "version": "2.40.0", + "version": "2.41.0", "private": true, "description": "Stress tests that can only run against the local server", "homepage": "https://fluidframework.com", @@ -76,7 +76,6 @@ "@fluidframework/id-compressor": "workspace:~", "@fluidframework/local-driver": "workspace:~", "@fluidframework/map": "workspace:~", - "@fluidframework/matrix": "workspace:~", "@fluidframework/runtime-definitions": "workspace:~", "@fluidframework/runtime-utils": "workspace:~", "@fluidframework/sequence": "workspace:~", @@ -106,8 +105,7 @@ "^tsc", "^api-extractor:commonjs", "@fluidframework/sequence#build:test", - "@fluidframework/map#build:test", - "@fluidframework/matrix#build:test" + "@fluidframework/map#build:test" ] } }, diff --git a/packages/test/local-server-stress-tests/src/ddsModels.ts b/packages/test/local-server-stress-tests/src/ddsModels.ts index 5f3ca9f908d1..7b2df9db91a1 100644 --- a/packages/test/local-server-stress-tests/src/ddsModels.ts +++ b/packages/test/local-server-stress-tests/src/ddsModels.ts @@ -8,8 +8,6 @@ import { DDSFuzzModel, DDSFuzzTestState } from "@fluid-private/test-dds-utils"; import type { IChannelFactory } from "@fluidframework/datastore-definitions/internal"; // eslint-disable-next-line import/no-internal-modules import { baseMapModel, baseDirModel } from "@fluidframework/map/internal/test"; -// eslint-disable-next-line import/no-internal-modules -import { baseSharedMatrixModel } from "@fluidframework/matrix/internal/test"; import { baseSharedStringModel, baseIntervalModel, @@ -67,5 +65,4 @@ export const ddsModelMap = generateSubModelMap( baseDirModel, baseSharedStringModel, baseIntervalModel, - baseSharedMatrixModel, ); diff --git a/packages/test/local-server-stress-tests/src/stressDataObject.ts b/packages/test/local-server-stress-tests/src/stressDataObject.ts index 773fe932ba6a..f8646c641032 100644 --- a/packages/test/local-server-stress-tests/src/stressDataObject.ts +++ b/packages/test/local-server-stress-tests/src/stressDataObject.ts @@ -78,6 +78,9 @@ export class StressDataObject extends DataObject { registryEntries: [ ["StressDataObject", new LazyPromise(async () => StressDataObject.factory)], ], + policies: { + readonlyInStagingMode: false, + }, }); get StressDataObject() { @@ -348,8 +351,6 @@ export const createRuntimeFactory = (): IRuntimeFactory => { return this; }, instantiateRuntime: async (context, existing) => { - // This can be removed or scoped to options passed to specific data stores once we support squashing more widely. - context.options.allowStagingModeWithoutSquashing = true; const runtime = await loadContainerRuntime({ context, existing, diff --git a/packages/test/local-server-stress-tests/src/test/localServerStress.spec.ts b/packages/test/local-server-stress-tests/src/test/localServerStress.spec.ts index d1ab80309f20..6191f7092ce0 100644 --- a/packages/test/local-server-stress-tests/src/test/localServerStress.spec.ts +++ b/packages/test/local-server-stress-tests/src/test/localServerStress.spec.ts @@ -29,10 +29,13 @@ describe("Local Server Stress", () => { saveFailures, // saveSuccesses, skip: [ - ...[0, 13, 45, 56], // Number of keys not same - ...[30], // Number of subDirectories not same, - ...[99], // Rollback op does not match last pending - ...[8, 67], // Client closes due to id compressor related asserts in a fatal codepath + ...[15], // 0x54e + ...[39], // 0xa6f + ...[5, 22, 36], // Number of keys not same + ...[6], // channel maps should be the same size + ...[7], // Number of subDirectories not same, + ...[12], // Rollback op does not match last pending + ...[31, 58, 62, 87], // Client closes due to id compressor related asserts in a fatal codepath ], }); }); diff --git a/packages/test/local-server-stress-tests/src/test/localServerStressOrderSequentially.spec.ts b/packages/test/local-server-stress-tests/src/test/localServerStressOrderSequentially.spec.ts index 75a9019c3947..0f4c858f2ea3 100644 --- a/packages/test/local-server-stress-tests/src/test/localServerStressOrderSequentially.spec.ts +++ b/packages/test/local-server-stress-tests/src/test/localServerStressOrderSequentially.spec.ts @@ -92,13 +92,10 @@ describe("Local Server Stress with rollback", () => { // saveSuccesses, configurations: { "Fluid.ContainerRuntime.EnableRollback": true }, skip: [ - ...[15], // timeout - ...[61, 82], // Mismatch in pending changes - ...[66], // interval start side not equal - ...[76], // Rollback op does not match last pending - ...[84, 88], // Startpoints of interval different - ...[12, 28, 32, 36, 44, 45, 55, 60, 89], // Number of subDirectories not same - ...[4, 14, 69, 74], // 0xb86 and 0xb89: exit staging mode logic failing due to id compressor allocation ops + ...[1], // closed container: 0xb85 + ...[47], // Mismatch in pending changes + ...[15, 23, 51, 63, 65], // Number of subDirectories/keys not same + ...[13], // 0xb86 and 0xb89: exit staging mode logic failing due to id compressor allocation ops ], }); }); From 7eb5cbf596816dd2763a11467903e0f095a552b5 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Wed, 14 May 2025 09:56:19 -0700 Subject: [PATCH 04/27] export source maps --- packages/dds/matrix/src/test/tsconfig.json | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/dds/matrix/src/test/tsconfig.json b/packages/dds/matrix/src/test/tsconfig.json index ce8ecfa14737..0fbd1f9a40d7 100644 --- a/packages/dds/matrix/src/test/tsconfig.json +++ b/packages/dds/matrix/src/test/tsconfig.json @@ -6,6 +6,8 @@ "types": ["mocha", "node"], "noUnusedLocals": false, // Need it so memory tests can declare local variables just for the sake of keeping things in memory "noUncheckedIndexedAccess": false, + "declaration": true, + "declarationMap": true, }, "include": ["./**/*"], "references": [ From 6219d37e315e662fb37b4fd9a3b35de7f86181a1 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Wed, 14 May 2025 11:55:53 -0700 Subject: [PATCH 05/27] fix attw --- packages/dds/matrix/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dds/matrix/package.json b/packages/dds/matrix/package.json index 72a25eb0944b..6011c29fe267 100644 --- a/packages/dds/matrix/package.json +++ b/packages/dds/matrix/package.json @@ -76,7 +76,7 @@ "build:test": "npm run build:test:esm && npm run build:test:cjs", "build:test:cjs": "fluid-tsc commonjs --project ./src/test/tsconfig.cjs.json", "build:test:esm": "tsc --project ./src/test/tsconfig.json", - "check:are-the-types-wrong": "attw --pack .", + "check:are-the-types-wrong": "attw --pack . --exclude-entrypoints ./internal/test", "check:biome": "biome check .", "check:exports": "concurrently \"npm:check:exports:*\"", "check:exports:bundle-release-tags": "api-extractor run --config api-extractor/api-extractor-lint-bundle.json", From 90a2dd9f410652f463ac0645cf0919b148ecd5bd Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Fri, 16 May 2025 14:05:13 -0700 Subject: [PATCH 06/27] use undefined rather than -1 --- packages/dds/matrix/src/matrix.ts | 43 +++++++++++++++---------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index 052a6d1a85c0..5c79b604b769 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -246,7 +246,7 @@ export class SharedMatrix private readonly pending = new SparseArray2D(); // Tracks pending writes. private cellLastWriteTracker = new SparseArray2D(); // Tracks last writes sequence number and clientId in a cell. // Tracks the seq number of Op at which policy switch happens from Last Write Win to First Write Win. - private setCellLwwToFwwPolicySwitchOpSeqNumber: number; + private setCellLwwToFwwPolicySwitchOpSeqNumber: number | undefined; private userSwitchedSetCellPolicy = false; // Set to true when the user calls switchPolicy. // Used to track if there is any reentrancy in setCell code. @@ -267,7 +267,6 @@ export class SharedMatrix ) { super(id, runtime, attributes, "fluid_matrix_"); - this.setCellLwwToFwwPolicySwitchOpSeqNumber = -1; this.rows = new PermutationVector( SnapshotPath.rows, this.logger, @@ -333,7 +332,10 @@ export class SharedMatrix } public isSetCellConflictResolutionPolicyFWW(): boolean { - return this.setCellLwwToFwwPolicySwitchOpSeqNumber > -1 || this.userSwitchedSetCellPolicy; + return ( + this.setCellLwwToFwwPolicySwitchOpSeqNumber !== undefined || + this.userSwitchedSetCellPolicy + ); } public getCell(row: number, col: number): MatrixItem { @@ -471,7 +473,8 @@ export class SharedMatrix col, value, fwwMode: - this.userSwitchedSetCellPolicy || this.setCellLwwToFwwPolicySwitchOpSeqNumber > -1, + this.userSwitchedSetCellPolicy || + this.setCellLwwToFwwPolicySwitchOpSeqNumber !== undefined, }; const rowsRef = this.createOpMetadataLocalRef(this.rows, row, localSeq); @@ -675,7 +678,7 @@ export class SharedMatrix ]; // Only need to store it in the snapshot if we have switched the policy already. - if (this.setCellLwwToFwwPolicySwitchOpSeqNumber > -1) { + if (this.setCellLwwToFwwPolicySwitchOpSeqNumber !== undefined) { artifactsToSummarize.push(this.cellLastWriteTracker.snapshot()); } builder.addBlob( @@ -796,7 +799,7 @@ export class SharedMatrix // otherwise raise conflict. We want to check the current mode here and not that // whether op was made in FWW or not. if ( - this.setCellLwwToFwwPolicySwitchOpSeqNumber === -1 || + this.setCellLwwToFwwPolicySwitchOpSeqNumber === undefined || lastCellModificationDetails === undefined || referenceSeqNumber >= lastCellModificationDetails.seqNum ) { @@ -855,7 +858,7 @@ export class SharedMatrix this.cells = SparseArray2D.load(cellData); this.setCellLwwToFwwPolicySwitchOpSeqNumber = - setCellLwwToFwwPolicySwitchOpSeqNumber ?? -1; + setCellLwwToFwwPolicySwitchOpSeqNumber ?? undefined; if (cellLastWriteTracker !== undefined) { this.cellLastWriteTracker = SparseArray2D.load(cellLastWriteTracker); } @@ -874,7 +877,7 @@ export class SharedMatrix message: ISequencedDocumentMessage, ): boolean { assert( - this.setCellLwwToFwwPolicySwitchOpSeqNumber > -1, + this.setCellLwwToFwwPolicySwitchOpSeqNumber !== undefined, 0x85f /* should be in Fww mode when calling this method */, ); assert(message.clientId !== null, 0x860 /* clientId should not be null */); @@ -927,11 +930,10 @@ export class SharedMatrix ); const { row, col, value, fwwMode } = contents; - const isPreviousSetCellPolicyModeFWW = - this.setCellLwwToFwwPolicySwitchOpSeqNumber > -1; + const isPreviousSetCellPolicyModeFWW = this.setCellLwwToFwwPolicySwitchOpSeqNumber; // If this is the first op notifying us of the policy change, then set the policy change seq number. - if (this.setCellLwwToFwwPolicySwitchOpSeqNumber === -1 && fwwMode === true) { - this.setCellLwwToFwwPolicySwitchOpSeqNumber = msg.sequenceNumber; + if (fwwMode === true) { + this.setCellLwwToFwwPolicySwitchOpSeqNumber ??= msg.sequenceNumber; } assert(msg.clientId !== null, 0x861 /* clientId should not be null!! */); @@ -945,9 +947,9 @@ export class SharedMatrix // If policy is switched and cell should be modified too based on policy, then update the tracker. // If policy is not switched, then also update the tracker in case it is the latest. if ( - (this.setCellLwwToFwwPolicySwitchOpSeqNumber > -1 && + (this.setCellLwwToFwwPolicySwitchOpSeqNumber !== undefined && this.shouldSetCellBasedOnFWW(rowHandle, colHandle, msg)) || - (this.setCellLwwToFwwPolicySwitchOpSeqNumber === -1 && isLatestPendingOp) + (this.setCellLwwToFwwPolicySwitchOpSeqNumber === undefined && isLatestPendingOp) ) { this.cellLastWriteTracker.setCell(rowHandle, colHandle, { seqNum: msg.sequenceNumber, @@ -971,12 +973,12 @@ export class SharedMatrix isHandleValid(rowHandle) && isHandleValid(colHandle), 0x022 /* "SharedMatrix row and/or col handles are invalid!" */, ); - if (this.setCellLwwToFwwPolicySwitchOpSeqNumber > -1) { + if (this.setCellLwwToFwwPolicySwitchOpSeqNumber !== undefined) { // If someone tried to Overwrite the cell value or first write on this cell or // same client tried to modify the cell or if the previous mode was LWW, then we need to still // overwrite the cell and raise conflict if we have pending changes as our change is going to be lost. if ( - !isPreviousSetCellPolicyModeFWW || + isPreviousSetCellPolicyModeFWW === undefined || this.shouldSetCellBasedOnFWW(rowHandle, colHandle, msg) ) { const previousValue = this.cells.getCell(rowHandle, colHandle); @@ -1064,12 +1066,9 @@ export class SharedMatrix }; public switchSetCellPolicy(): void { - if (this.setCellLwwToFwwPolicySwitchOpSeqNumber === -1) { - if (this.isAttached()) { - this.userSwitchedSetCellPolicy = true; - } else { - this.setCellLwwToFwwPolicySwitchOpSeqNumber = 0; - } + this.userSwitchedSetCellPolicy = true; + if (!this.isAttached()) { + this.setCellLwwToFwwPolicySwitchOpSeqNumber ??= 0; } } From 35882b104b475ea59665b5c19e6bd4bddc79ec87 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Fri, 16 May 2025 14:17:15 -0700 Subject: [PATCH 07/27] combine fwwPolicy --- packages/dds/matrix/src/matrix.ts | 56 ++++++++++++++++++------------- 1 file changed, 32 insertions(+), 24 deletions(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index 5c79b604b769..a084040d7eb0 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -245,9 +245,10 @@ export class SharedMatrix private cells = new SparseArray2D>(); // Stores cell values. private readonly pending = new SparseArray2D(); // Tracks pending writes. private cellLastWriteTracker = new SparseArray2D(); // Tracks last writes sequence number and clientId in a cell. - // Tracks the seq number of Op at which policy switch happens from Last Write Win to First Write Win. - private setCellLwwToFwwPolicySwitchOpSeqNumber: number | undefined; - private userSwitchedSetCellPolicy = false; // Set to true when the user calls switchPolicy. + + private fwwPolicy: + | { enabled: false; switchOpSeqNumber?: undefined } + | { enabled: true; switchOpSeqNumber: number | undefined } = { enabled: false }; // Set to true when the user calls switchPolicy. // Used to track if there is any reentrancy in setCell code. private reentrantCount: number = 0; @@ -332,10 +333,7 @@ export class SharedMatrix } public isSetCellConflictResolutionPolicyFWW(): boolean { - return ( - this.setCellLwwToFwwPolicySwitchOpSeqNumber !== undefined || - this.userSwitchedSetCellPolicy - ); + return this.fwwPolicy.enabled; } public getCell(row: number, col: number): MatrixItem { @@ -472,9 +470,7 @@ export class SharedMatrix row, col, value, - fwwMode: - this.userSwitchedSetCellPolicy || - this.setCellLwwToFwwPolicySwitchOpSeqNumber !== undefined, + fwwMode: this.fwwPolicy.enabled, }; const rowsRef = this.createOpMetadataLocalRef(this.rows, row, localSeq); @@ -674,11 +670,11 @@ export class SharedMatrix const artifactsToSummarize = [ this.cells.snapshot(), this.pending.snapshot(), - this.setCellLwwToFwwPolicySwitchOpSeqNumber, + this.fwwPolicy.switchOpSeqNumber, ]; // Only need to store it in the snapshot if we have switched the policy already. - if (this.setCellLwwToFwwPolicySwitchOpSeqNumber !== undefined) { + if (this.fwwPolicy.enabled) { artifactsToSummarize.push(this.cellLastWriteTracker.snapshot()); } builder.addBlob( @@ -799,7 +795,7 @@ export class SharedMatrix // otherwise raise conflict. We want to check the current mode here and not that // whether op was made in FWW or not. if ( - this.setCellLwwToFwwPolicySwitchOpSeqNumber === undefined || + this.fwwPolicy.switchOpSeqNumber === undefined || lastCellModificationDetails === undefined || referenceSeqNumber >= lastCellModificationDetails.seqNum ) { @@ -857,8 +853,15 @@ export class SharedMatrix ]; this.cells = SparseArray2D.load(cellData); - this.setCellLwwToFwwPolicySwitchOpSeqNumber = - setCellLwwToFwwPolicySwitchOpSeqNumber ?? undefined; + this.fwwPolicy = + setCellLwwToFwwPolicySwitchOpSeqNumber === undefined + ? { + enabled: false, + } + : { + enabled: true, + switchOpSeqNumber: setCellLwwToFwwPolicySwitchOpSeqNumber ?? undefined, + }; if (cellLastWriteTracker !== undefined) { this.cellLastWriteTracker = SparseArray2D.load(cellLastWriteTracker); } @@ -877,7 +880,7 @@ export class SharedMatrix message: ISequencedDocumentMessage, ): boolean { assert( - this.setCellLwwToFwwPolicySwitchOpSeqNumber !== undefined, + this.fwwPolicy.switchOpSeqNumber !== undefined, 0x85f /* should be in Fww mode when calling this method */, ); assert(message.clientId !== null, 0x860 /* clientId should not be null */); @@ -930,10 +933,13 @@ export class SharedMatrix ); const { row, col, value, fwwMode } = contents; - const isPreviousSetCellPolicyModeFWW = this.setCellLwwToFwwPolicySwitchOpSeqNumber; + const isPreviousSetCellPolicyModeFWW = this.fwwPolicy.switchOpSeqNumber; // If this is the first op notifying us of the policy change, then set the policy change seq number. if (fwwMode === true) { - this.setCellLwwToFwwPolicySwitchOpSeqNumber ??= msg.sequenceNumber; + this.fwwPolicy = { + enabled: true, + switchOpSeqNumber: this.fwwPolicy.switchOpSeqNumber ?? msg.sequenceNumber, + }; } assert(msg.clientId !== null, 0x861 /* clientId should not be null!! */); @@ -947,9 +953,9 @@ export class SharedMatrix // If policy is switched and cell should be modified too based on policy, then update the tracker. // If policy is not switched, then also update the tracker in case it is the latest. if ( - (this.setCellLwwToFwwPolicySwitchOpSeqNumber !== undefined && + (this.fwwPolicy.switchOpSeqNumber !== undefined && this.shouldSetCellBasedOnFWW(rowHandle, colHandle, msg)) || - (this.setCellLwwToFwwPolicySwitchOpSeqNumber === undefined && isLatestPendingOp) + (this.fwwPolicy.switchOpSeqNumber === undefined && isLatestPendingOp) ) { this.cellLastWriteTracker.setCell(rowHandle, colHandle, { seqNum: msg.sequenceNumber, @@ -973,7 +979,7 @@ export class SharedMatrix isHandleValid(rowHandle) && isHandleValid(colHandle), 0x022 /* "SharedMatrix row and/or col handles are invalid!" */, ); - if (this.setCellLwwToFwwPolicySwitchOpSeqNumber !== undefined) { + if (this.fwwPolicy.switchOpSeqNumber !== undefined) { // If someone tried to Overwrite the cell value or first write on this cell or // same client tried to modify the cell or if the previous mode was LWW, then we need to still // overwrite the cell and raise conflict if we have pending changes as our change is going to be lost. @@ -1066,9 +1072,11 @@ export class SharedMatrix }; public switchSetCellPolicy(): void { - this.userSwitchedSetCellPolicy = true; - if (!this.isAttached()) { - this.setCellLwwToFwwPolicySwitchOpSeqNumber ??= 0; + if (!this.fwwPolicy.enabled) { + this.fwwPolicy = { + enabled: true, + switchOpSeqNumber: this.isAttached() ? undefined : 0, + }; } } From fdd4196132c5fd09bd7cb6333b209b4e9f60cda2 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Mon, 19 May 2025 09:40:55 -0700 Subject: [PATCH 08/27] fix backcompat --- packages/dds/matrix/src/matrix.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index a084040d7eb0..a79cf972624d 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -670,7 +670,8 @@ export class SharedMatrix const artifactsToSummarize = [ this.cells.snapshot(), this.pending.snapshot(), - this.fwwPolicy.switchOpSeqNumber, + // back-compat: used -1 for disabled + this.fwwPolicy.switchOpSeqNumber ?? -1, ]; // Only need to store it in the snapshot if we have switched the policy already. @@ -853,14 +854,16 @@ export class SharedMatrix ]; this.cells = SparseArray2D.load(cellData); + // back-compat: used -1 for disabled, also may not exist + const switchOpSeqNumber = setCellLwwToFwwPolicySwitchOpSeqNumber ?? -1; this.fwwPolicy = - setCellLwwToFwwPolicySwitchOpSeqNumber === undefined + switchOpSeqNumber === -1 ? { enabled: false, } : { enabled: true, - switchOpSeqNumber: setCellLwwToFwwPolicySwitchOpSeqNumber ?? undefined, + switchOpSeqNumber, }; if (cellLastWriteTracker !== undefined) { this.cellLastWriteTracker = SparseArray2D.load(cellLastWriteTracker); From 4d989a0d6ee010b318850217483077929438556a Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Mon, 19 May 2025 10:01:00 -0700 Subject: [PATCH 09/27] move fww to state --- packages/dds/matrix/src/matrix.ts | 33 +++++++++++++++++-------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index a79cf972624d..5f5962ea88aa 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -247,8 +247,8 @@ export class SharedMatrix private cellLastWriteTracker = new SparseArray2D(); // Tracks last writes sequence number and clientId in a cell. private fwwPolicy: - | { enabled: false; switchOpSeqNumber?: undefined } - | { enabled: true; switchOpSeqNumber: number | undefined } = { enabled: false }; // Set to true when the user calls switchPolicy. + | { state: "off"; switchOpSeqNumber?: undefined } + | { state: "on"; switchOpSeqNumber: number | undefined } = { state: "off" }; // Set to true when the user calls switchPolicy. // Used to track if there is any reentrancy in setCell code. private reentrantCount: number = 0; @@ -333,7 +333,7 @@ export class SharedMatrix } public isSetCellConflictResolutionPolicyFWW(): boolean { - return this.fwwPolicy.enabled; + return this.fwwPolicy.state !== "off"; } public getCell(row: number, col: number): MatrixItem { @@ -470,7 +470,7 @@ export class SharedMatrix row, col, value, - fwwMode: this.fwwPolicy.enabled, + fwwMode: this.fwwPolicy.state !== "off", }; const rowsRef = this.createOpMetadataLocalRef(this.rows, row, localSeq); @@ -671,11 +671,11 @@ export class SharedMatrix this.cells.snapshot(), this.pending.snapshot(), // back-compat: used -1 for disabled - this.fwwPolicy.switchOpSeqNumber ?? -1, + this.fwwPolicy.state === "on" ? this.fwwPolicy.switchOpSeqNumber : -1, ]; // Only need to store it in the snapshot if we have switched the policy already. - if (this.fwwPolicy.enabled) { + if (this.fwwPolicy.state === "on") { artifactsToSummarize.push(this.cellLastWriteTracker.snapshot()); } builder.addBlob( @@ -855,14 +855,17 @@ export class SharedMatrix this.cells = SparseArray2D.load(cellData); // back-compat: used -1 for disabled, also may not exist - const switchOpSeqNumber = setCellLwwToFwwPolicySwitchOpSeqNumber ?? -1; + const switchOpSeqNumber = + setCellLwwToFwwPolicySwitchOpSeqNumber === -1 + ? undefined + : (setCellLwwToFwwPolicySwitchOpSeqNumber ?? undefined); this.fwwPolicy = - switchOpSeqNumber === -1 + switchOpSeqNumber === undefined ? { - enabled: false, + state: "off", } : { - enabled: true, + state: "on", switchOpSeqNumber, }; if (cellLastWriteTracker !== undefined) { @@ -938,10 +941,10 @@ export class SharedMatrix const { row, col, value, fwwMode } = contents; const isPreviousSetCellPolicyModeFWW = this.fwwPolicy.switchOpSeqNumber; // If this is the first op notifying us of the policy change, then set the policy change seq number. - if (fwwMode === true) { + if (fwwMode === true && this.fwwPolicy.switchOpSeqNumber === undefined) { this.fwwPolicy = { - enabled: true, - switchOpSeqNumber: this.fwwPolicy.switchOpSeqNumber ?? msg.sequenceNumber, + state: "on", + switchOpSeqNumber: msg.sequenceNumber, }; } @@ -1075,9 +1078,9 @@ export class SharedMatrix }; public switchSetCellPolicy(): void { - if (!this.fwwPolicy.enabled) { + if (this.fwwPolicy.state === "off") { this.fwwPolicy = { - enabled: true, + state: "on", switchOpSeqNumber: this.isAttached() ? undefined : 0, }; } From 1e25590c9f5c150a44b7afdf1e6d7c400e79e864 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Mon, 19 May 2025 10:03:12 -0700 Subject: [PATCH 10/27] add local state --- packages/dds/matrix/src/matrix.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index 5f5962ea88aa..e750ec2f3358 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -248,7 +248,8 @@ export class SharedMatrix private fwwPolicy: | { state: "off"; switchOpSeqNumber?: undefined } - | { state: "on"; switchOpSeqNumber: number | undefined } = { state: "off" }; // Set to true when the user calls switchPolicy. + | { state: "local"; switchOpSeqNumber?: undefined } + | { state: "on"; switchOpSeqNumber: number } = { state: "off" }; // Set to true when the user calls switchPolicy. // Used to track if there is any reentrancy in setCell code. private reentrantCount: number = 0; @@ -1079,10 +1080,12 @@ export class SharedMatrix public switchSetCellPolicy(): void { if (this.fwwPolicy.state === "off") { - this.fwwPolicy = { - state: "on", - switchOpSeqNumber: this.isAttached() ? undefined : 0, - }; + this.fwwPolicy = this.isAttached() + ? { state: "local" } + : { + state: "on", + switchOpSeqNumber: 0, + }; } } From 8e5fc5ee9b0cd5880292a76c8ebde601a5c15fa1 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Mon, 19 May 2025 10:12:15 -0700 Subject: [PATCH 11/27] move cellLastWriteTracker to ffwPolicy --- packages/dds/matrix/src/matrix.ts | 43 ++++++++++++++++++------------- 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index e750ec2f3358..3eceb2e6149c 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -244,12 +244,17 @@ export class SharedMatrix private cells = new SparseArray2D>(); // Stores cell values. private readonly pending = new SparseArray2D(); // Tracks pending writes. - private cellLastWriteTracker = new SparseArray2D(); // Tracks last writes sequence number and clientId in a cell. private fwwPolicy: - | { state: "off"; switchOpSeqNumber?: undefined } - | { state: "local"; switchOpSeqNumber?: undefined } - | { state: "on"; switchOpSeqNumber: number } = { state: "off" }; // Set to true when the user calls switchPolicy. + | { state: "off"; switchOpSeqNumber?: undefined; cellLastWriteTracker?: undefined } + | { state: "local"; switchOpSeqNumber?: undefined; cellLastWriteTracker?: undefined } + | { + state: "on"; + switchOpSeqNumber: number; + cellLastWriteTracker: SparseArray2D; + } = { + state: "off", + }; // Set to true when the user calls switchPolicy. // Used to track if there is any reentrancy in setCell code. private reentrantCount: number = 0; @@ -677,7 +682,7 @@ export class SharedMatrix // Only need to store it in the snapshot if we have switched the policy already. if (this.fwwPolicy.state === "on") { - artifactsToSummarize.push(this.cellLastWriteTracker.snapshot()); + artifactsToSummarize.push(this.fwwPolicy.cellLastWriteTracker.snapshot()); } builder.addBlob( SnapshotPath.cells, @@ -787,7 +792,7 @@ export class SharedMatrix this.rows.removeLocalReferencePosition(rowsRef); this.cols.removeLocalReferencePosition(colsRef); if (row !== undefined && col !== undefined && row >= 0 && col >= 0) { - const lastCellModificationDetails = this.cellLastWriteTracker.getCell( + const lastCellModificationDetails = this.fwwPolicy.cellLastWriteTracker?.getCell( rowHandle, colHandle, ); @@ -868,10 +873,8 @@ export class SharedMatrix : { state: "on", switchOpSeqNumber, + cellLastWriteTracker: SparseArray2D.load(cellLastWriteTracker), }; - if (cellLastWriteTracker !== undefined) { - this.cellLastWriteTracker = SparseArray2D.load(cellLastWriteTracker); - } } catch (error) { this.logger.sendErrorEvent({ eventName: "MatrixLoadFailed" }, error); } @@ -891,7 +894,7 @@ export class SharedMatrix 0x85f /* should be in Fww mode when calling this method */, ); assert(message.clientId !== null, 0x860 /* clientId should not be null */); - const lastCellModificationDetails = this.cellLastWriteTracker.getCell( + const lastCellModificationDetails = this.fwwPolicy.cellLastWriteTracker?.getCell( rowHandle, colHandle, ); @@ -946,6 +949,7 @@ export class SharedMatrix this.fwwPolicy = { state: "on", switchOpSeqNumber: msg.sequenceNumber, + cellLastWriteTracker: new SparseArray2D(), }; } @@ -964,7 +968,7 @@ export class SharedMatrix this.shouldSetCellBasedOnFWW(rowHandle, colHandle, msg)) || (this.fwwPolicy.switchOpSeqNumber === undefined && isLatestPendingOp) ) { - this.cellLastWriteTracker.setCell(rowHandle, colHandle, { + this.fwwPolicy.cellLastWriteTracker?.setCell(rowHandle, colHandle, { seqNum: msg.sequenceNumber, clientId: msg.clientId, }); @@ -996,7 +1000,7 @@ export class SharedMatrix ) { const previousValue = this.cells.getCell(rowHandle, colHandle); this.cells.setCell(rowHandle, colHandle, value); - this.cellLastWriteTracker.setCell(rowHandle, colHandle, { + this.fwwPolicy.cellLastWriteTracker?.setCell(rowHandle, colHandle, { seqNum: msg.sequenceNumber, clientId: msg.clientId, }); @@ -1021,10 +1025,6 @@ export class SharedMatrix // If there is a pending (unACKed) local write to the same cell, skip the current op // since it "happened before" the pending write. this.cells.setCell(rowHandle, colHandle, value); - this.cellLastWriteTracker.setCell(rowHandle, colHandle, { - seqNum: msg.sequenceNumber, - clientId: msg.clientId, - }); for (const consumer of this.consumers.values()) { consumer.cellsChanged(adjustedRow, adjustedCol, 1, 1, this); } @@ -1066,7 +1066,10 @@ export class SharedMatrix for (const rowHandle of rowHandles) { this.cells.clearRows(/* rowStart: */ rowHandle, /* rowCount: */ 1); this.pending.clearRows(/* rowStart: */ rowHandle, /* rowCount: */ 1); - this.cellLastWriteTracker.clearRows(/* rowStart: */ rowHandle, /* rowCount: */ 1); + this.fwwPolicy.cellLastWriteTracker?.clearRows( + /* rowStart: */ rowHandle, + /* rowCount: */ 1, + ); } }; @@ -1074,7 +1077,10 @@ export class SharedMatrix for (const colHandle of colHandles) { this.cells.clearCols(/* colStart: */ colHandle, /* colCount: */ 1); this.pending.clearCols(/* colStart: */ colHandle, /* colCount: */ 1); - this.cellLastWriteTracker.clearCols(/* colStart: */ colHandle, /* colCount: */ 1); + this.fwwPolicy.cellLastWriteTracker?.clearCols( + /* colStart: */ colHandle, + /* colCount: */ 1, + ); } }; @@ -1085,6 +1091,7 @@ export class SharedMatrix : { state: "on", switchOpSeqNumber: 0, + cellLastWriteTracker: new SparseArray2D(), }; } } From ebd247a55a8d9ba2e8279327e6b3a1fc862c27b3 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Mon, 19 May 2025 10:20:21 -0700 Subject: [PATCH 12/27] clean up typing --- packages/dds/matrix/src/matrix.ts | 69 ++++++++++++++++--------------- 1 file changed, 35 insertions(+), 34 deletions(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index 3eceb2e6149c..81388e59f25e 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -204,6 +204,15 @@ export interface ISharedMatrix switchSetCellPolicy(): void; } +type FirstWriterWinsPolicy = + | { state: "off" } + | { state: "local" } + | { + state: "on"; + switchOpSeqNumber: number; + cellLastWriteTracker: SparseArray2D; + }; + /** * A SharedMatrix holds a rectangular 2D array of values. Supported operations * include setting values and inserting/removing rows and columns. @@ -245,16 +254,9 @@ export class SharedMatrix private cells = new SparseArray2D>(); // Stores cell values. private readonly pending = new SparseArray2D(); // Tracks pending writes. - private fwwPolicy: - | { state: "off"; switchOpSeqNumber?: undefined; cellLastWriteTracker?: undefined } - | { state: "local"; switchOpSeqNumber?: undefined; cellLastWriteTracker?: undefined } - | { - state: "on"; - switchOpSeqNumber: number; - cellLastWriteTracker: SparseArray2D; - } = { + private fwwPolicy: FirstWriterWinsPolicy = { state: "off", - }; // Set to true when the user calls switchPolicy. + }; // Used to track if there is any reentrancy in setCell code. private reentrantCount: number = 0; @@ -792,19 +794,15 @@ export class SharedMatrix this.rows.removeLocalReferencePosition(rowsRef); this.cols.removeLocalReferencePosition(colsRef); if (row !== undefined && col !== undefined && row >= 0 && col >= 0) { - const lastCellModificationDetails = this.fwwPolicy.cellLastWriteTracker?.getCell( - rowHandle, - colHandle, - ); // If the mode is LWW, then send the op. // Otherwise if the current mode is FWW and if we generated this op, after seeing the // last set op, or it is the first set op for the cell, then regenerate the op, // otherwise raise conflict. We want to check the current mode here and not that // whether op was made in FWW or not. if ( - this.fwwPolicy.switchOpSeqNumber === undefined || - lastCellModificationDetails === undefined || - referenceSeqNumber >= lastCellModificationDetails.seqNum + this.fwwPolicy.state !== "on" || + referenceSeqNumber >= + (this.fwwPolicy.cellLastWriteTracker.getCell(rowHandle, colHandle)?.seqNum ?? 0) ) { this.sendSetCellOp(row, col, setOp.value, rowHandle, colHandle, localSeq); } else if (this.pending.getCell(rowHandle, colHandle) !== undefined) { @@ -890,11 +888,11 @@ export class SharedMatrix message: ISequencedDocumentMessage, ): boolean { assert( - this.fwwPolicy.switchOpSeqNumber !== undefined, + this.fwwPolicy.state === "on", 0x85f /* should be in Fww mode when calling this method */, ); assert(message.clientId !== null, 0x860 /* clientId should not be null */); - const lastCellModificationDetails = this.fwwPolicy.cellLastWriteTracker?.getCell( + const lastCellModificationDetails = this.fwwPolicy.cellLastWriteTracker.getCell( rowHandle, colHandle, ); @@ -943,9 +941,9 @@ export class SharedMatrix ); const { row, col, value, fwwMode } = contents; - const isPreviousSetCellPolicyModeFWW = this.fwwPolicy.switchOpSeqNumber; + const isPreviousSetCellPolicyModeFWW = this.fwwPolicy.state; // If this is the first op notifying us of the policy change, then set the policy change seq number. - if (fwwMode === true && this.fwwPolicy.switchOpSeqNumber === undefined) { + if (fwwMode === true && this.fwwPolicy.state !== "on") { this.fwwPolicy = { state: "on", switchOpSeqNumber: msg.sequenceNumber, @@ -964,11 +962,10 @@ export class SharedMatrix // If policy is switched and cell should be modified too based on policy, then update the tracker. // If policy is not switched, then also update the tracker in case it is the latest. if ( - (this.fwwPolicy.switchOpSeqNumber !== undefined && - this.shouldSetCellBasedOnFWW(rowHandle, colHandle, msg)) || - (this.fwwPolicy.switchOpSeqNumber === undefined && isLatestPendingOp) + this.fwwPolicy.state === "on" && + this.shouldSetCellBasedOnFWW(rowHandle, colHandle, msg) ) { - this.fwwPolicy.cellLastWriteTracker?.setCell(rowHandle, colHandle, { + this.fwwPolicy.cellLastWriteTracker.setCell(rowHandle, colHandle, { seqNum: msg.sequenceNumber, clientId: msg.clientId, }); @@ -990,7 +987,7 @@ export class SharedMatrix isHandleValid(rowHandle) && isHandleValid(colHandle), 0x022 /* "SharedMatrix row and/or col handles are invalid!" */, ); - if (this.fwwPolicy.switchOpSeqNumber !== undefined) { + if (this.fwwPolicy.state === "on") { // If someone tried to Overwrite the cell value or first write on this cell or // same client tried to modify the cell or if the previous mode was LWW, then we need to still // overwrite the cell and raise conflict if we have pending changes as our change is going to be lost. @@ -1000,7 +997,7 @@ export class SharedMatrix ) { const previousValue = this.cells.getCell(rowHandle, colHandle); this.cells.setCell(rowHandle, colHandle, value); - this.fwwPolicy.cellLastWriteTracker?.setCell(rowHandle, colHandle, { + this.fwwPolicy.cellLastWriteTracker.setCell(rowHandle, colHandle, { seqNum: msg.sequenceNumber, clientId: msg.clientId, }); @@ -1066,10 +1063,12 @@ export class SharedMatrix for (const rowHandle of rowHandles) { this.cells.clearRows(/* rowStart: */ rowHandle, /* rowCount: */ 1); this.pending.clearRows(/* rowStart: */ rowHandle, /* rowCount: */ 1); - this.fwwPolicy.cellLastWriteTracker?.clearRows( - /* rowStart: */ rowHandle, - /* rowCount: */ 1, - ); + if (this.fwwPolicy.state === "on") { + this.fwwPolicy.cellLastWriteTracker?.clearRows( + /* rowStart: */ rowHandle, + /* rowCount: */ 1, + ); + } } }; @@ -1077,10 +1076,12 @@ export class SharedMatrix for (const colHandle of colHandles) { this.cells.clearCols(/* colStart: */ colHandle, /* colCount: */ 1); this.pending.clearCols(/* colStart: */ colHandle, /* colCount: */ 1); - this.fwwPolicy.cellLastWriteTracker?.clearCols( - /* colStart: */ colHandle, - /* colCount: */ 1, - ); + if (this.fwwPolicy.state === "on") { + this.fwwPolicy.cellLastWriteTracker?.clearCols( + /* colStart: */ colHandle, + /* colCount: */ 1, + ); + } } }; From a1a9a507e785a5608736c90de233b0747db598c3 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Mon, 19 May 2025 10:31:34 -0700 Subject: [PATCH 13/27] remove isPreviousSetCellPolicyModeFWW --- packages/dds/matrix/src/matrix.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index 81388e59f25e..ea33a0d378f0 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -941,7 +941,6 @@ export class SharedMatrix ); const { row, col, value, fwwMode } = contents; - const isPreviousSetCellPolicyModeFWW = this.fwwPolicy.state; // If this is the first op notifying us of the policy change, then set the policy change seq number. if (fwwMode === true && this.fwwPolicy.state !== "on") { this.fwwPolicy = { @@ -991,10 +990,7 @@ export class SharedMatrix // If someone tried to Overwrite the cell value or first write on this cell or // same client tried to modify the cell or if the previous mode was LWW, then we need to still // overwrite the cell and raise conflict if we have pending changes as our change is going to be lost. - if ( - isPreviousSetCellPolicyModeFWW === undefined || - this.shouldSetCellBasedOnFWW(rowHandle, colHandle, msg) - ) { + if (this.shouldSetCellBasedOnFWW(rowHandle, colHandle, msg)) { const previousValue = this.cells.getCell(rowHandle, colHandle); this.cells.setCell(rowHandle, colHandle, value); this.fwwPolicy.cellLastWriteTracker.setCell(rowHandle, colHandle, { From 8954fc3e9b68c47a3cc1da9f81674816ed622987 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Mon, 19 May 2025 10:40:46 -0700 Subject: [PATCH 14/27] cleanup summarize code --- packages/dds/matrix/src/matrix.ts | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index ea33a0d378f0..3f5017c41160 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -675,16 +675,21 @@ export class SharedMatrix SnapshotPath.cols, this.cols.summarize(this.runtime, this.handle, serializer), ); - const artifactsToSummarize = [ - this.cells.snapshot(), - this.pending.snapshot(), - // back-compat: used -1 for disabled - this.fwwPolicy.state === "on" ? this.fwwPolicy.switchOpSeqNumber : -1, - ]; + const artifactsToSummarize: ( + | undefined + | number + | ReturnType | number>["snapshot"]> + )[] = [this.cells.snapshot(), this.pending.snapshot()]; // Only need to store it in the snapshot if we have switched the policy already. if (this.fwwPolicy.state === "on") { - artifactsToSummarize.push(this.fwwPolicy.cellLastWriteTracker.snapshot()); + artifactsToSummarize.push( + this.fwwPolicy.switchOpSeqNumber, + this.fwwPolicy.cellLastWriteTracker.snapshot(), + ); + } else { + // back-compat: used -1 for disabled, and keep the number of items fixed by adding undefined for cellLastWriteTracker + artifactsToSummarize.push(-1, undefined); } builder.addBlob( SnapshotPath.cells, From 65eddccab9ec784174ff851c30ca042c059493d6 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Mon, 19 May 2025 12:28:11 -0700 Subject: [PATCH 15/27] work around snapshot test issue --- packages/dds/matrix/src/matrix.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index 3f5017c41160..95ad19a4c7d7 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -688,8 +688,11 @@ export class SharedMatrix this.fwwPolicy.cellLastWriteTracker.snapshot(), ); } else { - // back-compat: used -1 for disabled, and keep the number of items fixed by adding undefined for cellLastWriteTracker - artifactsToSummarize.push(-1, undefined); + // back-compat: used -1 for disabled + artifactsToSummarize.push( + -1, + // undefined - we should set undefined in place of cellLastWriteTracker to ensure the number of array entries is consistent, but this currently breaks snapshot tests + ); } builder.addBlob( SnapshotPath.cells, From 45c1c979524191984526e90ef9616bde20f3babf Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Mon, 19 May 2025 12:30:12 -0700 Subject: [PATCH 16/27] expand comments --- packages/dds/matrix/src/matrix.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index 95ad19a4c7d7..ed0bf81a2a41 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -691,7 +691,12 @@ export class SharedMatrix // back-compat: used -1 for disabled artifactsToSummarize.push( -1, - // undefined - we should set undefined in place of cellLastWriteTracker to ensure the number of array entries is consistent, but this currently breaks snapshot tests + /* + * we should set undefined in place of cellLastWriteTracker to ensure the number of array entries is consistent. + * Doing that currently breaks snapshot tests. Its is probably fine, but if new elements are ever added, we need + * ensure undefined is also set. + */ + // undefined ); } builder.addBlob( From 90509e1f24b48b7ec58447222772ddb0f5406c51 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Mon, 19 May 2025 16:01:45 -0700 Subject: [PATCH 17/27] Matrix: Make pending an array --- packages/dds/matrix/src/matrix.ts | 87 +++++++++++++------------------ 1 file changed, 37 insertions(+), 50 deletions(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index ed0bf81a2a41..1f5af13627aa 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -252,7 +252,7 @@ export class SharedMatrix private readonly cols: PermutationVector; // Map logical col to storage handle (if any) private cells = new SparseArray2D>(); // Stores cell values. - private readonly pending = new SparseArray2D(); // Tracks pending writes. + private readonly pending = new SparseArray2D(); // Tracks pending writes. private fwwPolicy: FirstWriterWinsPolicy = { state: "off", @@ -493,7 +493,9 @@ export class SharedMatrix }; this.submitLocalMessage(op, metadata); - this.pending.setCell(rowHandle, colHandle, localSeq); + const pending = this.pending.getCell(rowHandle, colHandle) ?? []; + pending.push(localSeq); + this.pending.setCell(rowHandle, colHandle, pending); } /** @@ -679,7 +681,16 @@ export class SharedMatrix | undefined | number | ReturnType | number>["snapshot"]> - )[] = [this.cells.snapshot(), this.pending.snapshot()]; + )[] = [ + this.cells.snapshot(), + /** + * we used to write this.pending.snapshot(). this should have never been done, as pending is only for local + * changes, and there should never be local changes in the summarizer. This was also never used on load + * as there is no way to understand a previous clients pending changes. so we just set this to a constant + * which matches an empty this.pending.snapshot() for back-compat in terms of the array length + */ + [undefined], + ]; // Only need to store it in the snapshot if we have switched the policy already. if (this.fwwPolicy.state === "on") { @@ -806,22 +817,28 @@ export class SharedMatrix const col = this.rebasePosition(this.cols, colsRef, localSeq); this.rows.removeLocalReferencePosition(rowsRef); this.cols.removeLocalReferencePosition(colsRef); - if (row !== undefined && col !== undefined && row >= 0 && col >= 0) { - // If the mode is LWW, then send the op. + + const pending = this.pending.getCell(rowHandle, colHandle); + assert(pending !== undefined, "local operation must have a pending array"); + const localSeqIndex = pending.indexOf(localSeq); + assert(localSeqIndex >= 0, "local operation must have a pending entry"); + const [pendingSeq] = pending.splice(localSeqIndex, 1); + assert(pendingSeq === localSeq, "must match"); + + if ( + row !== undefined && + col !== undefined && + row >= 0 && + col >= 0 && // If the mode is LWW, then send the op. // Otherwise if the current mode is FWW and if we generated this op, after seeing the // last set op, or it is the first set op for the cell, then regenerate the op, // otherwise raise conflict. We want to check the current mode here and not that // whether op was made in FWW or not. - if ( - this.fwwPolicy.state !== "on" || + (this.fwwPolicy.state !== "on" || referenceSeqNumber >= - (this.fwwPolicy.cellLastWriteTracker.getCell(rowHandle, colHandle)?.seqNum ?? 0) - ) { - this.sendSetCellOp(row, col, setOp.value, rowHandle, colHandle, localSeq); - } else if (this.pending.getCell(rowHandle, colHandle) !== undefined) { - // Clear the pending changes if any as we are not sending the op. - this.pending.setCell(rowHandle, colHandle, undefined); - } + (this.fwwPolicy.cellLastWriteTracker.getCell(rowHandle, colHandle)?.seqNum ?? 0)) + ) { + this.sendSetCellOp(row, col, setOp.value, rowHandle, colHandle, localSeq); } } else { switch (content.target) { @@ -968,9 +985,12 @@ export class SharedMatrix // We are receiving the ACK for a local pending set operation. const { rowHandle, colHandle, localSeq, rowsRef, colsRef } = localOpMetadata as ISetOpMetadata; - const isLatestPendingOp = this.isLatestPendingWrite(rowHandle, colHandle, localSeq); this.rows.removeLocalReferencePosition(rowsRef); this.cols.removeLocalReferencePosition(colsRef); + + const pending = this.pending.getCell(rowHandle, colHandle); + assert(pending?.shift() === localSeq, "must match"); + // If policy is switched and cell should be modified too based on policy, then update the tracker. // If policy is not switched, then also update the tracker in case it is the latest. if ( @@ -982,10 +1002,6 @@ export class SharedMatrix clientId: msg.clientId, }); } - - if (isLatestPendingOp) { - this.pending.setCell(rowHandle, colHandle, undefined); - } } else { const adjustedRow = this.rows.adjustPosition(row, msg); if (adjustedRow !== undefined) { @@ -1014,7 +1030,7 @@ export class SharedMatrix consumer.cellsChanged(adjustedRow, adjustedCol, 1, 1, this); } // Check is there are any pending changes, which will be rejected. If so raise conflict. - if (this.pending.getCell(rowHandle, colHandle) !== undefined) { + if ((this.pending.getCell(rowHandle, colHandle)?.length ?? 0) > 0) { // Don't reset the pending value yet, as there maybe more fww op from same client, so we want // to raise conflict event for that op also. this.emit( @@ -1027,7 +1043,7 @@ export class SharedMatrix ); } } - } else if (this.pending.getCell(rowHandle, colHandle) === undefined) { + } else if ((this.pending.getCell(rowHandle, colHandle)?.length ?? 0) === 0) { // If there is a pending (unACKed) local write to the same cell, skip the current op // since it "happened before" the pending write. this.cells.setCell(rowHandle, colHandle, value); @@ -1106,35 +1122,6 @@ export class SharedMatrix } } - /** - * Returns true if the latest pending write to the cell indicated by the given row/col handles - * matches the given 'localSeq'. - * - * A return value of `true` indicates that there are no later local operations queued that will - * clobber the write op at the given 'localSeq'. This includes later ops that overwrite the cell - * with a different value as well as row/col removals that might recycled the given row/col handles. - */ - private isLatestPendingWrite( - rowHandle: Handle, - colHandle: Handle, - localSeq: number, - ): boolean { - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const pendingLocalSeq = this.pending.getCell(rowHandle, colHandle)!; - - // Note while we're awaiting the ACK for a local set, it's possible for the row/col to be - // locally removed and the row/col handles recycled. If this happens, the pendingLocalSeq will - // be 'undefined' or > 'localSeq'. - assert( - !(pendingLocalSeq < localSeq), - 0x023 /* "The 'localSeq' of pending write (if any) must be <= the localSeq of the currently processed op." */, - ); - - // If this is the most recent write to the cell by the local client, the stored localSeq - // will be an exact match for the given 'localSeq'. - return pendingLocalSeq === localSeq; - } - public toString(): string { let s = `client:${ this.runtime.clientId From daef1fba2628f1283f474f2a1b1aa43bbaafe4ed Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Tue, 20 May 2025 13:31:00 -0700 Subject: [PATCH 18/27] most tests working --- packages/dds/matrix/src/matrix.ts | 110 ++++++++++++++---- .../local-server-stress-tests/package.json | 4 +- .../src/ddsModels.ts | 3 + ...localServerStressOrderSequentially.spec.ts | 1 + pnpm-lock.yaml | 73 ++++-------- 5 files changed, 118 insertions(+), 73 deletions(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index 1f5af13627aa..9eb4480ffa46 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -213,6 +213,11 @@ type FirstWriterWinsPolicy = cellLastWriteTracker: SparseArray2D; }; +interface PendingChanges { + local: { localSeq: number; value: MatrixItem }[]; + consensus?: MatrixItem | undefined; +} + /** * A SharedMatrix holds a rectangular 2D array of values. Supported operations * include setting values and inserting/removing rows and columns. @@ -252,7 +257,7 @@ export class SharedMatrix private readonly cols: PermutationVector; // Map logical col to storage handle (if any) private cells = new SparseArray2D>(); // Stores cell values. - private readonly pending = new SparseArray2D(); // Tracks pending writes. + private readonly pending = new SparseArray2D>(); // Tracks pending writes. private fwwPolicy: FirstWriterWinsPolicy = { state: "off", @@ -418,21 +423,22 @@ export class SharedMatrix value: MatrixItem, rowHandle = this.rows.getAllocatedHandle(row), colHandle = this.cols.getAllocatedHandle(col), + rollback?: boolean, ): void { this.protectAgainstReentrancy(() => { - if (this.undo !== undefined) { - let oldValue = this.cells.getCell(rowHandle, colHandle); - if (oldValue === null) { - oldValue = undefined; - } + const oldValue = this.cells.getCell(rowHandle, colHandle) ?? undefined; + if (this.undo !== undefined) { this.undo.cellSet(rowHandle, colHandle, oldValue); } this.cells.setCell(rowHandle, colHandle, value); - if (this.isAttached()) { - this.sendSetCellOp(row, col, value, rowHandle, colHandle); + if (this.isAttached() && rollback !== true) { + const pending = this.sendSetCellOp(row, col, value, rowHandle, colHandle); + if (pending.local.length === 1) { + pending.consensus ??= oldValue; + } } // Avoid reentrancy by raising change notifications after the op is queued. @@ -467,7 +473,7 @@ export class SharedMatrix rowHandle: Handle, colHandle: Handle, localSeq = this.nextLocalSeq(), - ): void { + ): PendingChanges { assert( this.isAttached(), 0x1e2 /* "Caller must ensure 'isAttached()' before calling 'sendSetCellOp'." */, @@ -493,9 +499,12 @@ export class SharedMatrix }; this.submitLocalMessage(op, metadata); - const pending = this.pending.getCell(rowHandle, colHandle) ?? []; - pending.push(localSeq); + const pending: PendingChanges = this.pending.getCell(rowHandle, colHandle) ?? { + local: [], + }; + pending.local.push({ localSeq, value }); this.pending.setCell(rowHandle, colHandle, pending); + return pending; } /** @@ -820,10 +829,12 @@ export class SharedMatrix const pending = this.pending.getCell(rowHandle, colHandle); assert(pending !== undefined, "local operation must have a pending array"); - const localSeqIndex = pending.indexOf(localSeq); + const { local } = pending; + assert(local !== undefined, "local operation must have a pending array"); + const localSeqIndex = local.findIndex((p) => p.localSeq === localSeq); assert(localSeqIndex >= 0, "local operation must have a pending entry"); - const [pendingSeq] = pending.splice(localSeqIndex, 1); - assert(pendingSeq === localSeq, "must match"); + const [change] = local.splice(localSeqIndex, 1); + assert(change.localSeq === localSeq, "must match"); if ( row !== undefined && @@ -857,6 +868,47 @@ export class SharedMatrix } } + protected rollback(content: unknown, localOpMetadata: unknown): void { + const contents = content as MatrixSetOrVectorOp; + const target = contents.target; + + switch (target) { + case SnapshotPath.cols: { + this.cols.rollback(content, localOpMetadata); + break; + } + case SnapshotPath.rows: { + this.rows.rollback(content, localOpMetadata); + break; + } + case undefined: { + assert(contents.type === MatrixOp.set, "only sets supported"); + const setMetadata = localOpMetadata as ISetOpMetadata; + + const pending = this.pending.getCell(setMetadata.rowHandle, setMetadata.colHandle); + assert(pending !== undefined, "must have pending"); + + const change = pending.local.pop(); + assert(change?.localSeq === setMetadata.localSeq, "must have change"); + + const previous = + pending.local.length > 0 + ? pending.local[pending.local.length - 1].value + : pending.consensus; + + this.setCellCore( + contents.row, + contents.col, + previous, + setMetadata.rowHandle, + setMetadata.colHandle, + true, + ); + } + default: + } + } + protected onDisconnect(): void {} /** @@ -989,7 +1041,11 @@ export class SharedMatrix this.cols.removeLocalReferencePosition(colsRef); const pending = this.pending.getCell(rowHandle, colHandle); - assert(pending?.shift() === localSeq, "must match"); + const ackedChange = pending?.local.shift(); + assert(ackedChange?.localSeq === localSeq, "must match"); + if (pending?.local.length === 0) { + this.pending.setCell(rowHandle, colHandle, undefined); + } // If policy is switched and cell should be modified too based on policy, then update the tracker. // If policy is not switched, then also update the tracker in case it is the latest. @@ -997,6 +1053,8 @@ export class SharedMatrix this.fwwPolicy.state === "on" && this.shouldSetCellBasedOnFWW(rowHandle, colHandle, msg) ) { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + pending!.consensus = ackedChange.value; this.fwwPolicy.cellLastWriteTracker.setCell(rowHandle, colHandle, { seqNum: msg.sequenceNumber, clientId: msg.clientId, @@ -1015,6 +1073,7 @@ export class SharedMatrix isHandleValid(rowHandle) && isHandleValid(colHandle), 0x022 /* "SharedMatrix row and/or col handles are invalid!" */, ); + const pending = this.pending.getCell(rowHandle, colHandle); if (this.fwwPolicy.state === "on") { // If someone tried to Overwrite the cell value or first write on this cell or // same client tried to modify the cell or if the previous mode was LWW, then we need to still @@ -1026,11 +1085,14 @@ export class SharedMatrix seqNum: msg.sequenceNumber, clientId: msg.clientId, }); + if (pending !== undefined) { + pending.consensus = value; + } for (const consumer of this.consumers.values()) { consumer.cellsChanged(adjustedRow, adjustedCol, 1, 1, this); } // Check is there are any pending changes, which will be rejected. If so raise conflict. - if ((this.pending.getCell(rowHandle, colHandle)?.length ?? 0) > 0) { + if (pending !== undefined && pending.local.length > 0) { // Don't reset the pending value yet, as there maybe more fww op from same client, so we want // to raise conflict event for that op also. this.emit( @@ -1043,12 +1105,16 @@ export class SharedMatrix ); } } - } else if ((this.pending.getCell(rowHandle, colHandle)?.length ?? 0) === 0) { - // If there is a pending (unACKed) local write to the same cell, skip the current op - // since it "happened before" the pending write. - this.cells.setCell(rowHandle, colHandle, value); - for (const consumer of this.consumers.values()) { - consumer.cellsChanged(adjustedRow, adjustedCol, 1, 1, this); + } else { + if (pending === undefined || pending.local.length === 0) { + // If there is a pending (unACKed) local write to the same cell, skip the current op + // since it "happened before" the pending write. + this.cells.setCell(rowHandle, colHandle, value); + for (const consumer of this.consumers.values()) { + consumer.cellsChanged(adjustedRow, adjustedCol, 1, 1, this); + } + } else { + pending.consensus = value; } } } diff --git a/packages/test/local-server-stress-tests/package.json b/packages/test/local-server-stress-tests/package.json index edaffa173ba2..0bd5d7dc153a 100644 --- a/packages/test/local-server-stress-tests/package.json +++ b/packages/test/local-server-stress-tests/package.json @@ -76,6 +76,7 @@ "@fluidframework/id-compressor": "workspace:~", "@fluidframework/local-driver": "workspace:~", "@fluidframework/map": "workspace:~", + "@fluidframework/matrix": "workspace:~", "@fluidframework/runtime-definitions": "workspace:~", "@fluidframework/runtime-utils": "workspace:~", "@fluidframework/sequence": "workspace:~", @@ -106,7 +107,8 @@ "^api-extractor:commonjs", "@fluidframework/id-compressor#build:test", "@fluidframework/sequence#build:test", - "@fluidframework/map#build:test" + "@fluidframework/map#build:test", + "@fluidframework/matrix#build:test" ] } }, diff --git a/packages/test/local-server-stress-tests/src/ddsModels.ts b/packages/test/local-server-stress-tests/src/ddsModels.ts index 7b2df9db91a1..5f3ca9f908d1 100644 --- a/packages/test/local-server-stress-tests/src/ddsModels.ts +++ b/packages/test/local-server-stress-tests/src/ddsModels.ts @@ -8,6 +8,8 @@ import { DDSFuzzModel, DDSFuzzTestState } from "@fluid-private/test-dds-utils"; import type { IChannelFactory } from "@fluidframework/datastore-definitions/internal"; // eslint-disable-next-line import/no-internal-modules import { baseMapModel, baseDirModel } from "@fluidframework/map/internal/test"; +// eslint-disable-next-line import/no-internal-modules +import { baseSharedMatrixModel } from "@fluidframework/matrix/internal/test"; import { baseSharedStringModel, baseIntervalModel, @@ -65,4 +67,5 @@ export const ddsModelMap = generateSubModelMap( baseDirModel, baseSharedStringModel, baseIntervalModel, + baseSharedMatrixModel, ); diff --git a/packages/test/local-server-stress-tests/src/test/localServerStressOrderSequentially.spec.ts b/packages/test/local-server-stress-tests/src/test/localServerStressOrderSequentially.spec.ts index 3d21dbb47ce1..3ee98fd9ca80 100644 --- a/packages/test/local-server-stress-tests/src/test/localServerStressOrderSequentially.spec.ts +++ b/packages/test/local-server-stress-tests/src/test/localServerStressOrderSequentially.spec.ts @@ -91,6 +91,7 @@ describe("Local Server Stress with rollback", () => { saveFailures, // saveSuccesses, configurations: { "Fluid.ContainerRuntime.EnableRollback": true }, + only: [91], skip: [ ...[12, 28, 30], // Key not found or value not matching key ...[15, 38, 51, 63], // Number of keys not same (directory) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index b99d6a3a6858..bfe55acfe298 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -42,7 +42,7 @@ importers: version: link:packages/tools/changelog-generator-wrapper '@fluid-tools/build-cli': specifier: ^0.55.0 - version: 0.55.0(@types/node@18.19.67)(encoding@0.1.13)(typescript@5.4.5)(webpack-cli@5.1.4) + version: 0.55.0(@types/node@22.10.1)(encoding@0.1.13)(typescript@5.4.5)(webpack-cli@5.1.4) '@fluid-tools/markdown-magic': specifier: workspace:~ version: link:tools/markdown-magic @@ -51,7 +51,7 @@ importers: version: 2.0.3 '@fluidframework/build-tools': specifier: ^0.55.0 - version: 0.55.0(@types/node@18.19.67) + version: 0.55.0(@types/node@22.10.1) '@fluidframework/eslint-config-fluid': specifier: ^5.7.3 version: 5.7.3(eslint@8.55.0)(typescript@5.4.5) @@ -60,7 +60,7 @@ importers: version: 1.0.195075 '@microsoft/api-documenter': specifier: ^7.21.6 - version: 7.26.2(@types/node@18.19.67) + version: 7.26.2(@types/node@22.10.1) '@microsoft/api-extractor': specifier: 7.52.8 version: 7.52.8(patch_hash=ldzfpsbo3oeejrejk775zxplmi)(@types/node@22.10.1) @@ -90,7 +90,7 @@ importers: version: 8.55.0 jest: specifier: ^29.6.2 - version: 29.7.0(@types/node@18.19.67)(babel-plugin-macros@3.1.0)(ts-node@10.9.2(@types/node@18.19.67)(typescript@5.4.5)) + version: 29.7.0(@types/node@22.10.1)(babel-plugin-macros@3.1.0)(ts-node@10.9.2(@types/node@22.10.1)(typescript@5.4.5)) mocha: specifier: ^10.8.2 version: 10.8.2 @@ -13644,6 +13644,9 @@ importers: '@fluidframework/map': specifier: workspace:~ version: link:../../dds/map + '@fluidframework/matrix': + specifier: workspace:~ + version: link:../../dds/matrix '@fluidframework/runtime-definitions': specifier: workspace:~ version: link:../../runtime/runtime-definitions @@ -32092,23 +32095,23 @@ snapshots: transitivePeerDependencies: - tslib - '@microsoft/api-documenter@7.26.2(@types/node@18.19.67)': + '@microsoft/api-documenter@7.26.2(@types/node@22.10.1)': dependencies: - '@microsoft/api-extractor-model': 7.30.0(@types/node@18.19.67) + '@microsoft/api-extractor-model': 7.30.0(@types/node@22.10.1) '@microsoft/tsdoc': 0.15.1 - '@rushstack/node-core-library': 5.10.0(@types/node@18.19.67) - '@rushstack/terminal': 0.14.3(@types/node@18.19.67) - '@rushstack/ts-command-line': 4.23.1(@types/node@18.19.67) + '@rushstack/node-core-library': 5.10.0(@types/node@22.10.1) + '@rushstack/terminal': 0.14.3(@types/node@22.10.1) + '@rushstack/ts-command-line': 4.23.1(@types/node@22.10.1) js-yaml: 3.13.1 resolve: 1.22.10 transitivePeerDependencies: - '@types/node' - '@microsoft/api-extractor-model@7.30.0(@types/node@18.19.67)': + '@microsoft/api-extractor-model@7.30.0(@types/node@22.10.1)': dependencies: '@microsoft/tsdoc': 0.15.1 '@microsoft/tsdoc-config': 0.17.1 - '@rushstack/node-core-library': 5.10.0(@types/node@18.19.67) + '@rushstack/node-core-library': 5.10.0(@types/node@22.10.1) transitivePeerDependencies: - '@types/node' @@ -32941,7 +32944,7 @@ snapshots: optionalDependencies: '@types/node': 18.19.67 - '@rushstack/node-core-library@5.10.0(@types/node@18.19.67)': + '@rushstack/node-core-library@5.10.0(@types/node@22.10.1)': dependencies: ajv: 8.13.0 ajv-draft-04: 1.0.0(ajv@8.13.0) @@ -32952,7 +32955,7 @@ snapshots: resolve: 1.22.10 semver: 7.5.4 optionalDependencies: - '@types/node': 18.19.67 + '@types/node': 22.10.1 '@rushstack/node-core-library@5.13.1(@types/node@18.19.67)': dependencies: @@ -32985,12 +32988,12 @@ snapshots: resolve: 1.22.10 strip-json-comments: 3.1.1 - '@rushstack/terminal@0.14.3(@types/node@18.19.67)': + '@rushstack/terminal@0.14.3(@types/node@22.10.1)': dependencies: - '@rushstack/node-core-library': 5.10.0(@types/node@18.19.67) + '@rushstack/node-core-library': 5.10.0(@types/node@22.10.1) supports-color: 8.1.1 optionalDependencies: - '@types/node': 18.19.67 + '@types/node': 22.10.1 '@rushstack/terminal@0.15.3(@types/node@18.19.67)': dependencies: @@ -33008,9 +33011,9 @@ snapshots: '@rushstack/tree-pattern@0.3.1': {} - '@rushstack/ts-command-line@4.23.1(@types/node@18.19.67)': + '@rushstack/ts-command-line@4.23.1(@types/node@22.10.1)': dependencies: - '@rushstack/terminal': 0.14.3(@types/node@18.19.67) + '@rushstack/terminal': 0.14.3(@types/node@22.10.1) '@types/argparse': 1.0.38 argparse: 1.0.10 string-argv: 0.3.2 @@ -34038,7 +34041,7 @@ snapshots: '@vvago/vale@3.11.2': dependencies: - axios: 1.8.4 + axios: 1.8.4(debug@4.4.0) rimraf: 5.0.10 tar: 6.2.1 unzipper: 0.10.14 @@ -34493,14 +34496,6 @@ snapshots: transitivePeerDependencies: - debug - axios@1.8.4: - dependencies: - follow-redirects: 1.15.9 - form-data: 4.0.1 - proxy-from-env: 1.1.0 - transitivePeerDependencies: - - debug - axios@1.8.4(debug@4.3.7): dependencies: follow-redirects: 1.15.9(debug@4.3.7) @@ -36903,8 +36898,6 @@ snapshots: fn.name@1.1.0: {} - follow-redirects@1.15.9: {} - follow-redirects@1.15.9(debug@4.3.7): optionalDependencies: debug: 4.3.7 @@ -37552,18 +37545,6 @@ snapshots: transitivePeerDependencies: - supports-color - http-proxy-middleware@2.0.7(@types/express@4.17.21): - dependencies: - '@types/http-proxy': 1.17.15 - http-proxy: 1.18.1 - is-glob: 4.0.3 - is-plain-obj: 3.0.0 - micromatch: 4.0.8 - optionalDependencies: - '@types/express': 4.17.21 - transitivePeerDependencies: - - debug - http-proxy-middleware@2.0.7(@types/express@4.17.21)(debug@4.4.0): dependencies: '@types/http-proxy': 1.17.15 @@ -37576,14 +37557,6 @@ snapshots: transitivePeerDependencies: - debug - http-proxy@1.18.1: - dependencies: - eventemitter3: 4.0.7 - follow-redirects: 1.15.9 - requires-port: 1.0.0 - transitivePeerDependencies: - - debug - http-proxy@1.18.1(debug@4.4.0): dependencies: eventemitter3: 4.0.7 @@ -43755,7 +43728,7 @@ snapshots: express: 4.21.2 graceful-fs: 4.2.11 html-entities: 2.5.2 - http-proxy-middleware: 2.0.7(@types/express@4.17.21) + http-proxy-middleware: 2.0.7(@types/express@4.17.21)(debug@4.4.0) ipaddr.js: 2.2.0 launch-editor: 2.9.1 open: 8.4.2 From f92bf5fa2c53d0612bbe239e3e84e6050ec2fc2b Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Tue, 20 May 2025 14:27:00 -0700 Subject: [PATCH 19/27] permutation vector fix --- packages/dds/matrix/src/permutationvector.ts | 10 ++++++---- .../test/localServerStressOrderSequentially.spec.ts | 1 - 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/dds/matrix/src/permutationvector.ts b/packages/dds/matrix/src/permutationvector.ts index aae2aaa5e924..0710e628d975 100644 --- a/packages/dds/matrix/src/permutationvector.ts +++ b/packages/dds/matrix/src/permutationvector.ts @@ -342,10 +342,12 @@ export class PermutationVector extends Client { case MergeTreeDeltaType.INSERT: { // Pass 1: Perform any internal maintenance first to avoid reentrancy. for (const { segment, position } of ranges) { - // HACK: We need to include the allocated handle in the segment's JSON representation - // for snapshots, but need to ignore the remote client's handle allocations when - // processing remote ops. - segment.reset(); + if (opArgs.rollback !== true) { + // HACK: We need to include the allocated handle in the segment's JSON representation + // for snapshots, but need to ignore the remote client's handle allocations when + // processing remote ops. + segment.reset(); + } this.handleCache.itemsChanged( position, diff --git a/packages/test/local-server-stress-tests/src/test/localServerStressOrderSequentially.spec.ts b/packages/test/local-server-stress-tests/src/test/localServerStressOrderSequentially.spec.ts index 3ee98fd9ca80..3d21dbb47ce1 100644 --- a/packages/test/local-server-stress-tests/src/test/localServerStressOrderSequentially.spec.ts +++ b/packages/test/local-server-stress-tests/src/test/localServerStressOrderSequentially.spec.ts @@ -91,7 +91,6 @@ describe("Local Server Stress with rollback", () => { saveFailures, // saveSuccesses, configurations: { "Fluid.ContainerRuntime.EnableRollback": true }, - only: [91], skip: [ ...[12, 28, 30], // Key not found or value not matching key ...[15, 38, 51, 63], // Number of keys not same (directory) From 4d2b8e46d965b00fd0f0c995bf19ca38eec2df83 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Wed, 21 May 2025 10:57:56 -0700 Subject: [PATCH 20/27] fix post remove allocation --- packages/dds/matrix/src/matrix.ts | 42 +++++++++-------- packages/dds/matrix/src/permutationvector.ts | 49 ++++++++++++++------ 2 files changed, 58 insertions(+), 33 deletions(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index 9eb4480ffa46..e34dc1c41104 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -1066,8 +1066,8 @@ export class SharedMatrix const adjustedCol = this.cols.adjustPosition(col, msg); if (adjustedCol !== undefined) { - const rowHandle = this.rows.getAllocatedHandle(adjustedRow); - const colHandle = this.cols.getAllocatedHandle(adjustedCol); + const rowHandle = adjustedRow.handle; + const colHandle = adjustedCol.handle; assert( isHandleValid(rowHandle) && isHandleValid(colHandle), @@ -1088,21 +1088,23 @@ export class SharedMatrix if (pending !== undefined) { pending.consensus = value; } - for (const consumer of this.consumers.values()) { - consumer.cellsChanged(adjustedRow, adjustedCol, 1, 1, this); - } - // Check is there are any pending changes, which will be rejected. If so raise conflict. - if (pending !== undefined && pending.local.length > 0) { - // Don't reset the pending value yet, as there maybe more fww op from same client, so we want - // to raise conflict event for that op also. - this.emit( - "conflict", - row, - col, - value, // Current value - previousValue, // Ignored local value - this, - ); + if (adjustedRow.pos !== undefined && adjustedCol.pos !== undefined) { + for (const consumer of this.consumers.values()) { + consumer.cellsChanged(adjustedRow.pos, adjustedCol.pos, 1, 1, this); + } + // Check is there are any pending changes, which will be rejected. If so raise conflict. + if (pending !== undefined && pending.local.length > 0) { + // Don't reset the pending value yet, as there maybe more fww op from same client, so we want + // to raise conflict event for that op also. + this.emit( + "conflict", + row, + col, + value, // Current value + previousValue, // Ignored local value + this, + ); + } } } } else { @@ -1110,8 +1112,10 @@ export class SharedMatrix // If there is a pending (unACKed) local write to the same cell, skip the current op // since it "happened before" the pending write. this.cells.setCell(rowHandle, colHandle, value); - for (const consumer of this.consumers.values()) { - consumer.cellsChanged(adjustedRow, adjustedCol, 1, 1, this); + if (adjustedRow.pos !== undefined && adjustedCol.pos !== undefined) { + for (const consumer of this.consumers.values()) { + consumer.cellsChanged(adjustedRow.pos, adjustedCol.pos, 1, 1, this); + } } } else { pending.consensus = value; diff --git a/packages/dds/matrix/src/permutationvector.ts b/packages/dds/matrix/src/permutationvector.ts index 0710e628d975..08e7e8fdeef7 100644 --- a/packages/dds/matrix/src/permutationvector.ts +++ b/packages/dds/matrix/src/permutationvector.ts @@ -18,7 +18,6 @@ import { IMergeTreeDeltaOpArgs, IMergeTreeMaintenanceCallbackArgs, ISegment, - ISegmentInternal, MergeTreeDeltaType, MergeTreeMaintenanceType, segmentIsRemoved, @@ -199,23 +198,42 @@ export class PermutationVector extends Client { } public adjustPosition( - pos: number, - op: Pick, - ): number | undefined { - const { segment, offset } = this.getContainingSegment(pos, { + posToAdjust: number, + op: ISequencedDocumentMessage, + ): { pos: number | undefined; handle: Handle } | undefined { + const { segment, offset } = this.getContainingSegment(posToAdjust, { referenceSequenceNumber: op.referenceSequenceNumber, clientId: op.clientId, }); - // Note that until the MergeTree GCs, the segment is still reachable via `getContainingSegment()` with - // a `refSeq` in the past. Prevent remote ops from accidentally allocating or using recycled handles - // by checking for the presence of 'removedSeq'. - if (segment === undefined || segmentIsRemoved(segment)) { - return undefined; - } + assert( + segment !== undefined && offset !== undefined, + "segment must be available for operations in the collab window", + ); - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - return this.getPosition(segment) + offset!; + if (segmentIsRemoved(segment)) { + if (!isHandleValid(segment.start)) { + this.applyMsg({ + ...op, + contents: { + pos1: posToAdjust, + pos2: posToAdjust + 1, + props: {}, + type: MergeTreeDeltaType.ANNOTATE, + }, + }); + assert(segment.cachedLength === 1, "must be length 1 to allocate handle"); + segment.start = this.handleTable.allocate(); + } + + return { handle: segment.start, pos: undefined }; + } else { + const pos = this.getPosition(segment) + offset; + return { + pos, + handle: this.getAllocatedHandle(pos), + }; + } } public handleToPosition(handle: Handle, localSeq = this.getCollabWindow().localSeq): number { @@ -387,7 +405,10 @@ export class PermutationVector extends Client { } break; } - + case MergeTreeDeltaType.ANNOTATE: { + // ignore + break; + } default: { throw new Error("Unhandled MergeTreeDeltaType"); } From efcf8e0fc36c32ba95d5dc3201f9e433cde0c6a0 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Wed, 21 May 2025 11:55:13 -0700 Subject: [PATCH 21/27] extend walkSegments --- packages/dds/matrix/src/permutationvector.ts | 26 +++++++++++--------- packages/dds/merge-tree/src/client.ts | 11 +++++++-- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/packages/dds/matrix/src/permutationvector.ts b/packages/dds/matrix/src/permutationvector.ts index 08e7e8fdeef7..36496844bd28 100644 --- a/packages/dds/matrix/src/permutationvector.ts +++ b/packages/dds/matrix/src/permutationvector.ts @@ -212,21 +212,23 @@ export class PermutationVector extends Client { ); if (segmentIsRemoved(segment)) { - if (!isHandleValid(segment.start)) { - this.applyMsg({ - ...op, - contents: { - pos1: posToAdjust, - pos2: posToAdjust + 1, - props: {}, - type: MergeTreeDeltaType.ANNOTATE, + let handle = segment.start; + if (!isHandleValid(handle)) { + this.walkSegments( + (s) => { + const asPerm = s as PermutationSegment; + asPerm.start = handle = this.handleTable.allocate(); + return true; }, - }); - assert(segment.cachedLength === 1, "must be length 1 to allocate handle"); - segment.start = this.handleTable.allocate(); + posToAdjust, + posToAdjust + 1, + /* accum: */ undefined, + /* splitRange: */ true, + op, + ); } - return { handle: segment.start, pos: undefined }; + return { handle, pos: undefined }; } else { const pos = this.getPosition(segment) + offset; return { diff --git a/packages/dds/merge-tree/src/client.ts b/packages/dds/merge-tree/src/client.ts index e320a311e84d..315b340c9426 100644 --- a/packages/dds/merge-tree/src/client.ts +++ b/packages/dds/merge-tree/src/client.ts @@ -363,6 +363,7 @@ export class Client extends TypedEventEmitter { end: number | undefined, accum: TClientData, splitRange?: boolean, + perspective?: Pick, ): void; public walkSegments( handler: ISegmentAction, @@ -370,6 +371,7 @@ export class Client extends TypedEventEmitter { end?: number, accum?: undefined, splitRange?: boolean, + perspective?: Pick, ): void; public walkSegments( handler: ISegmentAction, @@ -377,10 +379,13 @@ export class Client extends TypedEventEmitter { end: number | undefined, accum: TClientData, splitRange: boolean = false, + perspective?: Pick, ): void { this._mergeTree.mapRange( handler, - this._mergeTree.localPerspective, + perspective === undefined + ? this.getCollabWindow().localPerspective + : this.getOperationPerspective(perspective), accum, start, end, @@ -556,7 +561,9 @@ export class Client extends TypedEventEmitter { } private getOperationPerspective( - sequencedMessage: ISequencedDocumentMessage | undefined, + sequencedMessage: + | Pick + | undefined, ): Perspective { if (!sequencedMessage) { return this._mergeTree.localPerspective; From b598b7196b64ae42c3d54b9a682d3523c08b3671 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Wed, 21 May 2025 11:56:08 -0700 Subject: [PATCH 22/27] remove undefined case --- packages/dds/matrix/src/matrix.ts | 108 +++++++++---------- packages/dds/matrix/src/permutationvector.ts | 2 +- 2 files changed, 53 insertions(+), 57 deletions(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index e34dc1c41104..d9dac6ca7764 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -1062,65 +1062,61 @@ export class SharedMatrix } } else { const adjustedRow = this.rows.adjustPosition(row, msg); - if (adjustedRow !== undefined) { - const adjustedCol = this.cols.adjustPosition(col, msg); - - if (adjustedCol !== undefined) { - const rowHandle = adjustedRow.handle; - const colHandle = adjustedCol.handle; - - assert( - isHandleValid(rowHandle) && isHandleValid(colHandle), - 0x022 /* "SharedMatrix row and/or col handles are invalid!" */, - ); - const pending = this.pending.getCell(rowHandle, colHandle); - if (this.fwwPolicy.state === "on") { - // If someone tried to Overwrite the cell value or first write on this cell or - // same client tried to modify the cell or if the previous mode was LWW, then we need to still - // overwrite the cell and raise conflict if we have pending changes as our change is going to be lost. - if (this.shouldSetCellBasedOnFWW(rowHandle, colHandle, msg)) { - const previousValue = this.cells.getCell(rowHandle, colHandle); - this.cells.setCell(rowHandle, colHandle, value); - this.fwwPolicy.cellLastWriteTracker.setCell(rowHandle, colHandle, { - seqNum: msg.sequenceNumber, - clientId: msg.clientId, - }); - if (pending !== undefined) { - pending.consensus = value; - } - if (adjustedRow.pos !== undefined && adjustedCol.pos !== undefined) { - for (const consumer of this.consumers.values()) { - consumer.cellsChanged(adjustedRow.pos, adjustedCol.pos, 1, 1, this); - } - // Check is there are any pending changes, which will be rejected. If so raise conflict. - if (pending !== undefined && pending.local.length > 0) { - // Don't reset the pending value yet, as there maybe more fww op from same client, so we want - // to raise conflict event for that op also. - this.emit( - "conflict", - row, - col, - value, // Current value - previousValue, // Ignored local value - this, - ); - } - } + const adjustedCol = this.cols.adjustPosition(col, msg); + + const rowHandle = adjustedRow.handle; + const colHandle = adjustedCol.handle; + + assert( + isHandleValid(rowHandle) && isHandleValid(colHandle), + 0x022 /* "SharedMatrix row and/or col handles are invalid!" */, + ); + const pending = this.pending.getCell(rowHandle, colHandle); + if (this.fwwPolicy.state === "on") { + // If someone tried to Overwrite the cell value or first write on this cell or + // same client tried to modify the cell or if the previous mode was LWW, then we need to still + // overwrite the cell and raise conflict if we have pending changes as our change is going to be lost. + if (this.shouldSetCellBasedOnFWW(rowHandle, colHandle, msg)) { + const previousValue = this.cells.getCell(rowHandle, colHandle); + this.cells.setCell(rowHandle, colHandle, value); + this.fwwPolicy.cellLastWriteTracker.setCell(rowHandle, colHandle, { + seqNum: msg.sequenceNumber, + clientId: msg.clientId, + }); + if (pending !== undefined) { + pending.consensus = value; + } + if (adjustedRow.pos !== undefined && adjustedCol.pos !== undefined) { + for (const consumer of this.consumers.values()) { + consumer.cellsChanged(adjustedRow.pos, adjustedCol.pos, 1, 1, this); } - } else { - if (pending === undefined || pending.local.length === 0) { - // If there is a pending (unACKed) local write to the same cell, skip the current op - // since it "happened before" the pending write. - this.cells.setCell(rowHandle, colHandle, value); - if (adjustedRow.pos !== undefined && adjustedCol.pos !== undefined) { - for (const consumer of this.consumers.values()) { - consumer.cellsChanged(adjustedRow.pos, adjustedCol.pos, 1, 1, this); - } - } - } else { - pending.consensus = value; + // Check is there are any pending changes, which will be rejected. If so raise conflict. + if (pending !== undefined && pending.local.length > 0) { + // Don't reset the pending value yet, as there maybe more fww op from same client, so we want + // to raise conflict event for that op also. + this.emit( + "conflict", + row, + col, + value, // Current value + previousValue, // Ignored local value + this, + ); + } + } + } + } else { + if (pending === undefined || pending.local.length === 0) { + // If there is a pending (unACKed) local write to the same cell, skip the current op + // since it "happened before" the pending write. + this.cells.setCell(rowHandle, colHandle, value); + if (adjustedRow.pos !== undefined && adjustedCol.pos !== undefined) { + for (const consumer of this.consumers.values()) { + consumer.cellsChanged(adjustedRow.pos, adjustedCol.pos, 1, 1, this); } } + } else { + pending.consensus = value; } } } diff --git a/packages/dds/matrix/src/permutationvector.ts b/packages/dds/matrix/src/permutationvector.ts index 36496844bd28..3881dfbf9fea 100644 --- a/packages/dds/matrix/src/permutationvector.ts +++ b/packages/dds/matrix/src/permutationvector.ts @@ -200,7 +200,7 @@ export class PermutationVector extends Client { public adjustPosition( posToAdjust: number, op: ISequencedDocumentMessage, - ): { pos: number | undefined; handle: Handle } | undefined { + ): { pos: number | undefined; handle: Handle } { const { segment, offset } = this.getContainingSegment(posToAdjust, { referenceSequenceNumber: op.referenceSequenceNumber, clientId: op.clientId, From c132c939840b027466d47506b3d523479f4a6619 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Wed, 21 May 2025 11:57:29 -0700 Subject: [PATCH 23/27] Update packages/dds/matrix/src/permutationvector.ts --- packages/dds/matrix/src/permutationvector.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/dds/matrix/src/permutationvector.ts b/packages/dds/matrix/src/permutationvector.ts index 3881dfbf9fea..8a315d30fa6c 100644 --- a/packages/dds/matrix/src/permutationvector.ts +++ b/packages/dds/matrix/src/permutationvector.ts @@ -407,10 +407,6 @@ export class PermutationVector extends Client { } break; } - case MergeTreeDeltaType.ANNOTATE: { - // ignore - break; - } default: { throw new Error("Unhandled MergeTreeDeltaType"); } From 2db5dbc1531fa71d58491223cb02bea9ff340054 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Wed, 21 May 2025 12:05:39 -0700 Subject: [PATCH 24/27] comments and clean up --- packages/dds/matrix/src/matrix.ts | 63 ++++++++++++-------- packages/dds/matrix/src/permutationvector.ts | 7 ++- 2 files changed, 43 insertions(+), 27 deletions(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index d9dac6ca7764..1b91a066b46d 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -213,8 +213,19 @@ type FirstWriterWinsPolicy = cellLastWriteTracker: SparseArray2D; }; -interface PendingChanges { +/** + * Used to track pending local changes for a cell + */ +interface PendingCellChanges { + /** + * the local changes including the local seq, and the value set at that local seq + */ local: { localSeq: number; value: MatrixItem }[]; + /** + * the latest consensus value across all clients. + * this will either be a remote value or ack'd local + * value. + */ consensus?: MatrixItem | undefined; } @@ -257,7 +268,7 @@ export class SharedMatrix private readonly cols: PermutationVector; // Map logical col to storage handle (if any) private cells = new SparseArray2D>(); // Stores cell values. - private readonly pending = new SparseArray2D>(); // Tracks pending writes. + private readonly pending = new SparseArray2D>(); // Tracks pending writes. private fwwPolicy: FirstWriterWinsPolicy = { state: "off", @@ -473,7 +484,7 @@ export class SharedMatrix rowHandle: Handle, colHandle: Handle, localSeq = this.nextLocalSeq(), - ): PendingChanges { + ): PendingCellChanges { assert( this.isAttached(), 0x1e2 /* "Caller must ensure 'isAttached()' before calling 'sendSetCellOp'." */, @@ -499,12 +510,12 @@ export class SharedMatrix }; this.submitLocalMessage(op, metadata); - const pending: PendingChanges = this.pending.getCell(rowHandle, colHandle) ?? { + const pendingCell: PendingCellChanges = this.pending.getCell(rowHandle, colHandle) ?? { local: [], }; - pending.local.push({ localSeq, value }); - this.pending.setCell(rowHandle, colHandle, pending); - return pending; + pendingCell.local.push({ localSeq, value }); + this.pending.setCell(rowHandle, colHandle, pendingCell); + return pendingCell; } /** @@ -827,9 +838,9 @@ export class SharedMatrix this.rows.removeLocalReferencePosition(rowsRef); this.cols.removeLocalReferencePosition(colsRef); - const pending = this.pending.getCell(rowHandle, colHandle); - assert(pending !== undefined, "local operation must have a pending array"); - const { local } = pending; + const pendingCell = this.pending.getCell(rowHandle, colHandle); + assert(pendingCell !== undefined, "local operation must have a pending array"); + const { local } = pendingCell; assert(local !== undefined, "local operation must have a pending array"); const localSeqIndex = local.findIndex((p) => p.localSeq === localSeq); assert(localSeqIndex >= 0, "local operation must have a pending entry"); @@ -885,16 +896,16 @@ export class SharedMatrix assert(contents.type === MatrixOp.set, "only sets supported"); const setMetadata = localOpMetadata as ISetOpMetadata; - const pending = this.pending.getCell(setMetadata.rowHandle, setMetadata.colHandle); - assert(pending !== undefined, "must have pending"); + const pendingCell = this.pending.getCell(setMetadata.rowHandle, setMetadata.colHandle); + assert(pendingCell !== undefined, "must have pending"); - const change = pending.local.pop(); + const change = pendingCell.local.pop(); assert(change?.localSeq === setMetadata.localSeq, "must have change"); const previous = - pending.local.length > 0 - ? pending.local[pending.local.length - 1].value - : pending.consensus; + pendingCell.local.length > 0 + ? pendingCell.local[pendingCell.local.length - 1].value + : pendingCell.consensus; this.setCellCore( contents.row, @@ -1040,10 +1051,10 @@ export class SharedMatrix this.rows.removeLocalReferencePosition(rowsRef); this.cols.removeLocalReferencePosition(colsRef); - const pending = this.pending.getCell(rowHandle, colHandle); - const ackedChange = pending?.local.shift(); + const pendingCell = this.pending.getCell(rowHandle, colHandle); + const ackedChange = pendingCell?.local.shift(); assert(ackedChange?.localSeq === localSeq, "must match"); - if (pending?.local.length === 0) { + if (pendingCell?.local.length === 0) { this.pending.setCell(rowHandle, colHandle, undefined); } @@ -1054,7 +1065,7 @@ export class SharedMatrix this.shouldSetCellBasedOnFWW(rowHandle, colHandle, msg) ) { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - pending!.consensus = ackedChange.value; + pendingCell!.consensus = ackedChange.value; this.fwwPolicy.cellLastWriteTracker.setCell(rowHandle, colHandle, { seqNum: msg.sequenceNumber, clientId: msg.clientId, @@ -1071,7 +1082,7 @@ export class SharedMatrix isHandleValid(rowHandle) && isHandleValid(colHandle), 0x022 /* "SharedMatrix row and/or col handles are invalid!" */, ); - const pending = this.pending.getCell(rowHandle, colHandle); + const pendingCell = this.pending.getCell(rowHandle, colHandle); if (this.fwwPolicy.state === "on") { // If someone tried to Overwrite the cell value or first write on this cell or // same client tried to modify the cell or if the previous mode was LWW, then we need to still @@ -1083,15 +1094,15 @@ export class SharedMatrix seqNum: msg.sequenceNumber, clientId: msg.clientId, }); - if (pending !== undefined) { - pending.consensus = value; + if (pendingCell !== undefined) { + pendingCell.consensus = value; } if (adjustedRow.pos !== undefined && adjustedCol.pos !== undefined) { for (const consumer of this.consumers.values()) { consumer.cellsChanged(adjustedRow.pos, adjustedCol.pos, 1, 1, this); } // Check is there are any pending changes, which will be rejected. If so raise conflict. - if (pending !== undefined && pending.local.length > 0) { + if (pendingCell !== undefined && pendingCell.local.length > 0) { // Don't reset the pending value yet, as there maybe more fww op from same client, so we want // to raise conflict event for that op also. this.emit( @@ -1106,7 +1117,7 @@ export class SharedMatrix } } } else { - if (pending === undefined || pending.local.length === 0) { + if (pendingCell === undefined || pendingCell.local.length === 0) { // If there is a pending (unACKed) local write to the same cell, skip the current op // since it "happened before" the pending write. this.cells.setCell(rowHandle, colHandle, value); @@ -1116,7 +1127,7 @@ export class SharedMatrix } } } else { - pending.consensus = value; + pendingCell.consensus = value; } } } diff --git a/packages/dds/matrix/src/permutationvector.ts b/packages/dds/matrix/src/permutationvector.ts index 8a315d30fa6c..ce61d59a7c4c 100644 --- a/packages/dds/matrix/src/permutationvector.ts +++ b/packages/dds/matrix/src/permutationvector.ts @@ -212,6 +212,12 @@ export class PermutationVector extends Client { ); if (segmentIsRemoved(segment)) { + // this case is tricky. the segment which the row or column data is remove + // but an op before that remove references a cell. we still want to apply + // the op, as the row/col could become active again in the case where + // the remove was local and it get's rolled back. so we allocate a handle + // for the row/col if not allocated, but don't put it in the cache + // as the cache can only contain live positions. let handle = segment.start; if (!isHandleValid(handle)) { this.walkSegments( @@ -386,7 +392,6 @@ export class PermutationVector extends Client { } break; } - case MergeTreeDeltaType.REMOVE: { // Pass 1: Perform any internal maintenance first to avoid reentrancy. for (const { segment, position } of ranges) { From d75f7d39c8b4695f3dfef0fa529e7ed4bc85edd2 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Thu, 29 May 2025 09:44:13 -0700 Subject: [PATCH 25/27] unit tests --- .../matrix/src/test/matrix.rollback.spec.ts | 292 ++++++++++++++++++ .../test-runtime-utils.legacy.alpha.api.md | 3 + .../runtime/test-runtime-utils/src/mocks.ts | 21 +- 3 files changed, 315 insertions(+), 1 deletion(-) create mode 100644 packages/dds/matrix/src/test/matrix.rollback.spec.ts diff --git a/packages/dds/matrix/src/test/matrix.rollback.spec.ts b/packages/dds/matrix/src/test/matrix.rollback.spec.ts new file mode 100644 index 000000000000..e9d4d78fc424 --- /dev/null +++ b/packages/dds/matrix/src/test/matrix.rollback.spec.ts @@ -0,0 +1,292 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +import { strict as assert } from "node:assert"; + +import { FlushMode } from "@fluidframework/runtime-definitions/internal"; +import { + MockContainerRuntimeFactory, + MockFluidDataStoreRuntime, + MockStorage, +} from "@fluidframework/test-runtime-utils/internal"; + +import { extract, matrixFactory } from "./utils.js"; + +describe("SharedMatrix rollback", () => { + it("should rollback a setCell operation", () => { + const containerRuntimeFactory = new MockContainerRuntimeFactory({ + flushMode: FlushMode.TurnBased, + }); + const dataRuntime = new MockFluidDataStoreRuntime(); + const containerRuntime = containerRuntimeFactory.createContainerRuntime(dataRuntime); + const matrix = matrixFactory.create(dataRuntime, "A"); + matrix.connect({ + deltaConnection: dataRuntime.createDeltaConnection(), + objectStorage: new MockStorage(), + }); + + matrix.insertRows(0, 1); + matrix.insertCols(0, 1); + // Do not process messages yet, keep ops local + + // Initial state after insert + assert.deepEqual(extract(matrix), [[undefined]], "initial state after insert"); + + matrix.setCell(0, 0, 42); + assert.deepEqual(extract(matrix), [[42]], "after setCell(0, 0, 42)"); + + // Rollback all unacked changes using containerRuntime.rollback + containerRuntime.rollback?.(); + // Should revert to state after insert + assert.deepEqual(extract(matrix), [], "after rollback of setCell"); + + // Now process messages to ensure no-op + containerRuntime.flush(); + containerRuntimeFactory.processAllMessages(); + assert.deepEqual(extract(matrix), [], "after processAllMessages post-rollback"); + }); + + it("should rollback an insertCols operation", () => { + const containerRuntimeFactory = new MockContainerRuntimeFactory({ + flushMode: FlushMode.TurnBased, + }); + const dataRuntime = new MockFluidDataStoreRuntime(); + const containerRuntime = containerRuntimeFactory.createContainerRuntime(dataRuntime); + const matrix = matrixFactory.create(dataRuntime, "A"); + matrix.connect({ + deltaConnection: dataRuntime.createDeltaConnection(), + objectStorage: new MockStorage(), + }); + + matrix.insertRows(0, 1); + // Initial state after row insert + assert.deepEqual(extract(matrix), [[]], "initial state after row insert"); + + matrix.insertCols(0, 2); + assert.deepEqual(extract(matrix), [[undefined, undefined]], "after insertCols(0, 2)"); + + // Rollback all unacked changes using containerRuntime.rollback + containerRuntime.rollback?.(); + // Should revert to state after row insert + assert.deepEqual(extract(matrix), [], "after rollback of insertCols"); + + // Now process messages to ensure no-op + containerRuntime.flush(); + containerRuntimeFactory.processAllMessages(); + assert.deepEqual(extract(matrix), [], "after processAllMessages post-rollback"); + }); + + it("should rollback a removeCols operation", () => { + const containerRuntimeFactory = new MockContainerRuntimeFactory({ + flushMode: FlushMode.TurnBased, + }); + const dataRuntime = new MockFluidDataStoreRuntime(); + const containerRuntime = containerRuntimeFactory.createContainerRuntime(dataRuntime); + const matrix = matrixFactory.create(dataRuntime, "A"); + matrix.connect({ + deltaConnection: dataRuntime.createDeltaConnection(), + objectStorage: new MockStorage(), + }); + + matrix.insertRows(0, 1); + matrix.insertCols(0, 2); + matrix.setCell(0, 0, 1); + matrix.setCell(0, 1, 2); + containerRuntime.flush(); + // State after sets + assert.deepEqual(extract(matrix), [[1, 2]], "after setCell(0, 0, 1) and setCell(0, 1, 2)"); + + matrix.removeCols(0, 1); + assert.deepEqual(extract(matrix), [[2]], "after removeCols(0, 1)"); + + // Rollback all unacked changes using containerRuntime.rollback + containerRuntime.rollback?.(); + // Should revert to state after setCell + assert.deepEqual(extract(matrix), [[1, 2]], "after rollback of removeCols"); + + // Now process messages to ensure no-op + containerRuntime.flush(); + containerRuntimeFactory.processAllMessages(); + assert.deepEqual(extract(matrix), [[1, 2]], "after processAllMessages post-rollback"); + }); + + it("should rollback an insertRows operation", () => { + const containerRuntimeFactory = new MockContainerRuntimeFactory({ + flushMode: FlushMode.TurnBased, + }); + const dataRuntime = new MockFluidDataStoreRuntime(); + const containerRuntime = containerRuntimeFactory.createContainerRuntime(dataRuntime); + const matrix = matrixFactory.create(dataRuntime, "A"); + matrix.connect({ + deltaConnection: dataRuntime.createDeltaConnection(), + objectStorage: new MockStorage(), + }); + + // Initial state + assert.deepEqual(extract(matrix), [], "initial state"); + + matrix.insertRows(0, 2); + assert.deepEqual(extract(matrix), [[], []], "after insertRows(0, 2)"); + + containerRuntime.rollback?.(); + assert.deepEqual(extract(matrix), [], "after rollback of insertRows"); + }); + + it("should rollback a removeRows operation", () => { + const containerRuntimeFactory = new MockContainerRuntimeFactory({ + flushMode: FlushMode.TurnBased, + }); + const dataRuntime = new MockFluidDataStoreRuntime(); + const containerRuntime = containerRuntimeFactory.createContainerRuntime(dataRuntime); + const matrix = matrixFactory.create(dataRuntime, "A"); + matrix.connect({ + deltaConnection: dataRuntime.createDeltaConnection(), + objectStorage: new MockStorage(), + }); + + matrix.insertRows(0, 2); + matrix.insertCols(0, 1); + matrix.setCell(0, 0, 10); + matrix.setCell(1, 0, 20); + containerRuntime.flush(); + assert.deepEqual(extract(matrix), [[10], [20]], "after setCell"); + + matrix.removeRows(0, 1); + assert.deepEqual(extract(matrix), [[20]], "after removeRows(0, 1)"); + + containerRuntime.rollback?.(); + assert.deepEqual(extract(matrix), [[10], [20]], "after rollback of removeRows"); + }); + + it("should rollback multiple operations in sequence", () => { + const containerRuntimeFactory = new MockContainerRuntimeFactory({ + flushMode: FlushMode.TurnBased, + }); + const dataRuntime = new MockFluidDataStoreRuntime(); + const containerRuntime = containerRuntimeFactory.createContainerRuntime(dataRuntime); + const matrix = matrixFactory.create(dataRuntime, "A"); + matrix.connect({ + deltaConnection: dataRuntime.createDeltaConnection(), + objectStorage: new MockStorage(), + }); + + matrix.insertRows(0, 1); + matrix.insertCols(0, 1); + matrix.setCell(0, 0, 5); + containerRuntime.flush(); + assert.deepEqual(extract(matrix), [[5]], "after setCell"); + + matrix.insertCols(1, 1); + matrix.setCell(0, 1, 15); + assert.deepEqual(extract(matrix), [[5, 15]], "after insertCols and setCell"); + + containerRuntime.rollback?.(); + assert.deepEqual(extract(matrix), [[5]], "after rollback of multiple ops"); + }); + + it("should be a no-op if rollback is called with no pending changes", () => { + const containerRuntimeFactory = new MockContainerRuntimeFactory({ + flushMode: FlushMode.TurnBased, + }); + const dataRuntime = new MockFluidDataStoreRuntime(); + const containerRuntime = containerRuntimeFactory.createContainerRuntime(dataRuntime); + const matrix = matrixFactory.create(dataRuntime, "A"); + matrix.connect({ + deltaConnection: dataRuntime.createDeltaConnection(), + objectStorage: new MockStorage(), + }); + + matrix.insertRows(0, 1); + matrix.insertCols(0, 1); + containerRuntime.flush(); + assert.deepEqual(extract(matrix), [[undefined]], "after flush"); + + containerRuntime.rollback?.(); + assert.deepEqual(extract(matrix), [[undefined]], "rollback with no pending changes"); + }); + + it("should not rollback already flushed (acked) operations", () => { + const containerRuntimeFactory = new MockContainerRuntimeFactory({ + flushMode: FlushMode.TurnBased, + }); + const dataRuntime = new MockFluidDataStoreRuntime(); + const containerRuntime = containerRuntimeFactory.createContainerRuntime(dataRuntime); + const matrix = matrixFactory.create(dataRuntime, "A"); + matrix.connect({ + deltaConnection: dataRuntime.createDeltaConnection(), + objectStorage: new MockStorage(), + }); + + matrix.insertRows(0, 1); + matrix.insertCols(0, 1); + containerRuntime.flush(); + containerRuntimeFactory.processAllMessages(); + assert.deepEqual(extract(matrix), [[undefined]], "after flush and process"); + + containerRuntime.rollback?.(); + assert.deepEqual(extract(matrix), [[undefined]], "rollback after flush (no effect)"); + }); + + it("should rollback with interleaved operations", () => { + const containerRuntimeFactory = new MockContainerRuntimeFactory({ + flushMode: FlushMode.TurnBased, + }); + const dataRuntime = new MockFluidDataStoreRuntime(); + const containerRuntime = containerRuntimeFactory.createContainerRuntime(dataRuntime); + const matrix = matrixFactory.create(dataRuntime, "A"); + matrix.connect({ + deltaConnection: dataRuntime.createDeltaConnection(), + objectStorage: new MockStorage(), + }); + + // Start with empty matrix + assert.deepEqual(extract(matrix), [], "initial state"); + + // Insert 2 rows and 2 columns + matrix.insertRows(0, 2); + matrix.insertCols(0, 2); + assert.deepEqual( + extract(matrix), + [ + [undefined, undefined], + [undefined, undefined], + ], + "after insertRows and insertCols", + ); + + // Set some cells + matrix.setCell(0, 0, 1); + matrix.setCell(1, 1, 2); + assert.deepEqual( + extract(matrix), + [ + [1, undefined], + [undefined, 2], + ], + "after setCell", + ); + + // Remove a column + matrix.removeCols(0, 1); + assert.deepEqual(extract(matrix), [[undefined], [2]], "after removeCols(0, 1)"); + + // Insert a row + matrix.insertRows(1, 1); + assert.deepEqual( + extract(matrix), + [[undefined], [undefined], [2]], + "after insertRows(1, 1)", + ); + + // Set a cell in the new row + matrix.setCell(1, 0, 99); + assert.deepEqual(extract(matrix), [[undefined], [99], [2]], "after setCell(1, 0, 99)"); + + // Rollback all unacked changes + containerRuntime.rollback?.(); + // Should revert to initial state (empty matrix) + assert.deepEqual(extract(matrix), [], "after rollback of interleaved operations"); + }); +}); diff --git a/packages/runtime/test-runtime-utils/api-report/test-runtime-utils.legacy.alpha.api.md b/packages/runtime/test-runtime-utils/api-report/test-runtime-utils.legacy.alpha.api.md index 8fed129f99fd..6d1acd6461ea 100644 --- a/packages/runtime/test-runtime-utils/api-report/test-runtime-utils.legacy.alpha.api.md +++ b/packages/runtime/test-runtime-utils/api-report/test-runtime-utils.legacy.alpha.api.md @@ -96,6 +96,7 @@ export class MockContainerRuntime extends TypedEventEmitter; // (undocumented) submit(messageContent: any, localOpMetadata?: unknown): number; @@ -189,6 +190,8 @@ export class MockDeltaConnection implements IDeltaConnection { // (undocumented) reSubmit(content: any, localOpMetadata: unknown): void; // (undocumented) + rollback?(message: any, localOpMetadata: unknown): void; + // (undocumented) setConnectionState(connected: boolean): void; // (undocumented) submit(messageContent: any, localOpMetadata: unknown): number; diff --git a/packages/runtime/test-runtime-utils/src/mocks.ts b/packages/runtime/test-runtime-utils/src/mocks.ts index 3db4d1daa3a1..b88011def464 100644 --- a/packages/runtime/test-runtime-utils/src/mocks.ts +++ b/packages/runtime/test-runtime-utils/src/mocks.ts @@ -121,6 +121,10 @@ export class MockDeltaConnection implements IDeltaConnection { public applyStashedOp(content: any): unknown { return this.handler?.applyStashedOp(content); } + + public rollback?(message: any, localOpMetadata: unknown): void { + this.handler?.rollback?.(message, localOpMetadata); + } } // Represents the structure of a pending message stored by the MockContainerRuntime. @@ -402,6 +406,19 @@ export class MockContainerRuntime extends TypedEventEmitter { + this.dataStoreRuntime.rollback?.(pm.content, pm.localOpMetadata); + }); + } + private generateIdAllocationOp(): IInternalMockRuntimeMessage | undefined { const idRange = this.dataStoreRuntime.idCompressor?.takeNextCreationRange(); if (idRange?.ids !== undefined) { @@ -1153,7 +1170,9 @@ export class MockFluidDataStoreRuntime } public rollback?(message: any, localOpMetadata: unknown): void { - return; + this.deltaConnections.forEach((dc) => { + dc.rollback?.(message, localOpMetadata); + }); } } From a17520859441969790b8d5dd9060170a6ec0aec7 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Thu, 29 May 2025 13:29:15 -0700 Subject: [PATCH 26/27] Apply suggestions from code review Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com> --- packages/dds/matrix/src/matrix.ts | 6 +++--- packages/runtime/test-runtime-utils/src/mocks.ts | 4 +++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index 1b91a066b46d..64d0ea759ca9 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -214,15 +214,15 @@ type FirstWriterWinsPolicy = }; /** - * Used to track pending local changes for a cell + * Tracks pending local changes for a cell. */ interface PendingCellChanges { /** - * the local changes including the local seq, and the value set at that local seq + * The local changes including the local seq, and the value set at that local seq. */ local: { localSeq: number; value: MatrixItem }[]; /** - * the latest consensus value across all clients. + * The latest consensus value across all clients. * this will either be a remote value or ack'd local * value. */ diff --git a/packages/runtime/test-runtime-utils/src/mocks.ts b/packages/runtime/test-runtime-utils/src/mocks.ts index b88011def464..fe7f8ffa6041 100644 --- a/packages/runtime/test-runtime-utils/src/mocks.ts +++ b/packages/runtime/test-runtime-utils/src/mocks.ts @@ -407,7 +407,9 @@ export class MockContainerRuntime extends TypedEventEmitter Date: Thu, 29 May 2025 13:29:49 -0700 Subject: [PATCH 27/27] remove undefined --- packages/dds/matrix/src/matrix.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dds/matrix/src/matrix.ts b/packages/dds/matrix/src/matrix.ts index 1b91a066b46d..8bef8f591f52 100644 --- a/packages/dds/matrix/src/matrix.ts +++ b/packages/dds/matrix/src/matrix.ts @@ -226,7 +226,7 @@ interface PendingCellChanges { * this will either be a remote value or ack'd local * value. */ - consensus?: MatrixItem | undefined; + consensus?: MatrixItem; } /**