Skip to content

Matrix: Enable rollback and local server stress #24604

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

anthony-murphy
Copy link
Contributor

@anthony-murphy anthony-murphy commented May 13, 2025

This pull request introduces significant changes to the SharedMatrix implementation to improve handling of pending cell changes and rollback operations, updates related utility classes, and enhances testing. Below is a summary of the most important changes grouped by theme.

Enhancements to SharedMatrix functionality:

  • Introduced a new PendingCellChanges<T> interface to track both local changes and the latest consensus value for a cell. This replaces the previous approach of tracking pending writes with a single number. (matrix.ts, [1] [2]
  • Modified the sendSetCellOp method to return PendingCellChanges<T> instead of a single localSeq value, enabling more granular tracking of pending operations. (matrix.ts, [1] [2]
  • Added a rollback method to handle undoing operations, ensuring that the matrix state can revert to the previous value or consensus value when necessary. (matrix.ts, packages/dds/matrix/src/matrix.tsR882-R922)

Improvements to operation handling:

  • Updated the logic for processing ACKs to correctly handle and clean up pending changes, ensuring that the consensus value is updated and remaining pending changes are managed appropriately. (matrix.ts, [1] [2]
  • Adjusted how adjustPosition in PermutationVector handles removed segments, allocating handles for rows/columns that may be reactivated during rollbacks. (permutationvector.ts, [1] [2]

Code cleanup and refactoring:

  • Removed the isLatestPendingWrite method from SharedMatrix, as its functionality is now integrated into the improved pending changes tracking system. (matrix.ts, packages/dds/matrix/src/matrix.tsL1109-L1137)
  • Simplified and clarified logic in PermutationVector for handling INSERT and REMOVE operations, including rollback scenarios. (permutationvector.ts, [1] [2]

Testing and dependencies:

  • Added @fluidframework/matrix as a dependency in the local-server-stress-tests package and integrated the baseSharedMatrixModel into the stress tests to ensure thorough validation of the updated SharedMatrix behavior. (package.json, [1] [2]

These changes collectively enhance the robustness and maintainability of the SharedMatrix component, particularly in scenarios involving concurrent edits, rollbacks, and acknowledgments.

@Copilot Copilot AI review requested due to automatic review settings May 13, 2025 17:14
@anthony-murphy anthony-murphy requested a review from a team as a code owner May 13, 2025 17:14
@github-actions github-actions bot added area: dds Issues related to distributed data structures area: tests Tests to add, test infrastructure improvements, etc dependencies Pull requests that update a dependency file labels May 13, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request improves the SharedMatrix implementation by adding rollback support, updating the pending operations tracking to handle multiple pending operations, and introducing a fuzz testing framework to validate the matrix behavior under stress.

  • Added rollback capabilities in SharedMatrix and updated the setCellCore method to handle rollback scenarios.
  • Modified pending operation storage from a single value to arrays for better multi-operation tracking.
  • Introduced a new fuzz testing framework and updated test configurations/package.json accordingly.

Reviewed Changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/test/local-server-stress-tests/src/test/localServerStressOrderSequentially.spec.ts Adjusted test skip settings for rollback scenarios.
packages/test/local-server-stress-tests/src/test/localServerStress.spec.ts Updated skip configuration to include additional test IDs.
packages/test/local-server-stress-tests/src/ddsModels.ts Added import for baseSharedMatrixModel to support matrix tests.
packages/test/local-server-stress-tests/package.json Added dependency and build/test configuration for matrix support.
packages/dds/matrix/src/test/matrix.fuzz.spec.ts Replaced custom fuzz model setup with baseSharedMatrixModel integration.
packages/dds/matrix/src/test/index.ts New export to expose the shared matrix test model.
packages/dds/matrix/src/test/fuzz.ts New file implementing the fuzz testing framework for SharedMatrix.
packages/dds/matrix/src/permutationvector.ts Inserted rollback check to prevent unnecessary handle allocation during rollback ops.
packages/dds/matrix/src/matrix.ts Updated rollback handling, pending op tracking, and added a dedicated rollback method.
packages/dds/matrix/package.json Updated package configuration to include internal test exports for the matrix module.
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

@github-actions github-actions bot added the base: main PRs targeted against main branch label May 13, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances the SharedMatrix implementation by introducing rollback support, improving pending operation tracking, and adding a fuzz testing framework.

  • Implements a rollback mechanism in SharedMatrix with changes to setCellCore and sendSetCellOp.
  • Updates the pending operation tracking by switching from a single number to an array of sequence numbers.
  • Adds new fuzz tests and updates package configuration for local server stress testing.

Reviewed Changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/test/local-server-stress-tests/src/test/localServerStressOrderSequentially.spec.ts Adjusts test skip configuration for rollback stress tests.
packages/test/local-server-stress-tests/src/test/localServerStress.spec.ts Updates skip arrays for stress test cases.
packages/test/local-server-stress-tests/src/ddsModels.ts Adds import for baseSharedMatrixModel to support new fuzz tests.
packages/test/local-server-stress-tests/package.json Includes the matrix package and its build test in test configuration.
packages/dds/matrix/src/test/matrix.fuzz.spec.ts Refactors fuzz tests to use baseSharedMatrixModel.
packages/dds/matrix/src/test/index.ts Exports the new baseSharedMatrixModel for reuse in test suites.
packages/dds/matrix/src/test/fuzz.ts Introduces a comprehensive fuzz testing framework for SharedMatrix.
packages/dds/matrix/src/permutationvector.ts Wraps segment JSON maintenance in a rollback check.
packages/dds/matrix/src/matrix.ts Implements rollback support and updates pending operation handling via arrays.
packages/dds/matrix/package.json Updates internal test exports configuration for matrix testing.
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

@github-actions github-actions bot removed the dependencies Pull requests that update a dependency file label May 19, 2025
@anthony-murphy anthony-murphy marked this pull request as draft May 19, 2025 20:43
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label May 20, 2025
@anthony-murphy anthony-murphy marked this pull request as ready for review May 21, 2025 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds Issues related to distributed data structures area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch dependencies Pull requests that update a dependency file Feature_StagingMode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant