-
Notifications
You must be signed in to change notification settings - Fork 532
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
Fix SharedTreeCore detached sequence numbers when loading into a detached tree #22947
base: main
Are you sure you want to change the base?
Conversation
|
||
// First, finish loading the edit manager so that we can inspect the sequence numbers of the commits on the trunk. | ||
await loadEditManager; | ||
const trunk = this.editManager.getSummaryData().trunk; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unnecessary to generate the entire summary here - we only need to look at the trunk commits. However, since we think this is unlikely to be hit in production I did this because it's easy/clean. If we ever found ourselves needing to optimize this path it would be trivial to expose some more specific data from the EditManager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Coverage Summary
↓ packages.dds.tree.src.shared-tree-core:
Line Coverage Change: -0.03% Branch Coverage Change: -0.74%
Metric Name | Baseline coverage | PR coverage | Coverage Diff |
---|---|---|---|
Branch Coverage | 91.53% | 90.79% | ↓ -0.74% |
Line Coverage | 97.43% | 97.40% | ↓ -0.03% |
Baseline commit: 522f76f
Baseline build: 303346
Happy Coding!!
Code coverage comparison check passed!!
026da0c
to
74bbbca
Compare
private getCommitSequenceId(trunkCommitOrTrunkBase: GraphCommit<TChangeset>): SequenceId { | ||
const id = this.trunkMetadata.get(trunkCommitOrTrunkBase.revision)?.sequenceId; | ||
if (id === undefined) { | ||
assert( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not strictly related to this PR, but I added this assert when debugging and think we should keep it. We don't want to return the minimumPossibleSequenceId
by accident (rather we only want to return it for the trunk base commit) - if we did, it'd probably be hard to debug.
Description
It is possible to load a summary into a detached SharedTree. In this case, the detached revision that is used to generate sequence IDs for commits while detached must be updated to ensure that it doesn't duplicate any of the sequence IDs already used in the summary. This PR fixes the issue and also adds an assert when sequencing in the EditManager to ensure that we don't sequence regressive sequence IDs.
This fix also allows us to trim the trunk when summarizing (without breaking our fuzz tests), which reduces summary sizes especially for detached trees with many synchronous edits.