Skip to content
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

Add explicit mechanism for preventing the submission of dangerous SharedTree commits #23276

Merged
merged 3 commits into from
Dec 17, 2024

Conversation

noencke
Copy link
Contributor

@noencke noencke commented Dec 10, 2024

Description

Currently, if the application of a local commit crashes a SharedTree client, that client will crash before sending the op to other clients, which prevents them from crashing in turn. This is good, but the way that it is currently achieved is somewhat obfuscated, relies on bizarre event ordering guarantees, and is preventing some future features from being possible.

More specifically, the checkout currently updates its forest in response to its branch's "beforeChange" event. This is because op submission happens in response to the "afterChange" event, so if the checkout crashes during "beforeChange", we won't progress to "afterChange". However, that means that when the end user of SharedTree receives a "nodeChanged" or "treeChanged" event, it will be in the context of the "beforeChange" event - so the forest will be updated according to their change, but the commit graph will not. Therefore, they cannot (sanely) do operations that affect the commit graph - like forking or merging their branches - in an event handler. This PR moves the forest update to the "afterChange" event, so that the commit graph is updated before the user's event handler is called. It does this by adding an explicit mechanism to the checkout for monitoring when commits have been "validated" - and SharedTree then uses this to determine when they should be submitted to other clients. SharedTreeCore now attempts to submit commits during "beforeChange", not "afterChange", but is intercepted by SharedTree and then delayed until after validation.

This PR also does a smattering of other related cleanup, including:

  • Removing the return values from all the branch operations, for simplicity.
  • Adjusting the arguments to "beforeBatch" and "afterBatch" to suit their usage and adding documentation.
  • Tightening the arguments to SharedTreeBranchChange and improving the documentation.
  • Adding a helper function for implementing lazy+cached properties, and using it to optimize the merge operation of SharedTreeBranch as well as for a cached property in the rebase logic.
  • Removing the current (and confusing) injection of the event emitter from the SharedTree into the checkout, and moving it into a more explicit load function. While still ugly, it is at least straightforward, and it combines with and cleans up the existing "on load" function setTipRevisionForLoadedData().

@Copilot Copilot bot review requested due to automatic review settings December 10, 2024 19:10
@noencke noencke requested a review from a team as a code owner December 10, 2024 19:10
@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: tree base: main PRs targeted against main branch labels Dec 10, 2024

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 6 changed files in this pull request and generated no suggestions.

Files not reviewed (1)
  • packages/dds/tree/src/shared-tree-core/index.ts: Evaluated as low risk
Comments skipped due to low confidence (1)

packages/dds/tree/src/shared-tree-core/sharedTreeCore.ts:311

  • The 'isResubmit' parameter is now required in the 'submitCommit' method, but there might be places where this method is called without providing this parameter. Ensure all calls to this method provide the 'isResubmit' parameter.
isResubmit: boolean,
@noencke noencke requested review from sonalideshpandemsft and removed request for sonalideshpandemsft December 10, 2024 20:17
@noencke noencke merged commit f1be6e3 into microsoft:main Dec 17, 2024
30 checks passed
@noencke noencke deleted the branch-cleanup branch December 17, 2024 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: tree area: dds Issues related to distributed data structures base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants