-
Notifications
You must be signed in to change notification settings - Fork 561
Added validation that clientSequenceNumbers are always increasing when receiving ops #26099
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR adds validation to detect non-contiguous clientSequenceNumbers when ops are received by the DeltaManager. The goal is to catch data corruption issues (such as duplicate ops being sequenced by the service) earlier in the processing pipeline with clearer error messages, rather than failing later with cryptic errors like assert 0x645 in the id compressor.
Key changes:
- Added clientSequenceNumber validation in DeltaManager to ensure monotonically increasing sequence numbers per client
- Added comprehensive test coverage for various scenarios including gaps, duplicates, null clientIds, and multi-client tracking
- Updated existing tests that had gaps in clientSequenceNumbers to expect the new validation error
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| packages/loader/container-loader/src/deltaManager.ts | Added SafeMessageProperties type, lastObservedMessageByClient map for tracking, and validateClientSequenceNumberConsistency method to check for contiguous clientSequenceNumbers |
| packages/loader/container-loader/src/test/deltaManager.spec.ts | Reorganized test structure, added new test suite for Client Sequence Number Validation with multiple test cases, and updated existing tests to expect the new validation errors |
| /** | ||
| * Properties of a message that are safe to log for debugging purposes. | ||
| */ | ||
| type SafeMessageProperties = Pick< |
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.
i think this type and comment could lead to issues, as the object stored in lastObservedMessageByClient isn't safe to log, as it the whole message stored. i think it would be better to just store ISequencedDocumentMessage or reused the helper extractSafePropertiesFromMessage which will actually create a new object with only safe properties
Added validation that the clientSequenceNumbers for a given client are always increasing. Currently, if the clientSequenceNumber for a client is repeated (say, the same op is sequenced twice by the service), it doesn't fail when the op is received in delta manager. It instead fails later when the op is processed for consumption. For exampls, we have been seeing failures with assert 0x645 which is thrown by id compressor when it sees these duplicate ops.
With this validation, these failures should be deteced as soon as the op is received by the delta manager with a much clear data corruption error because the document is corrupted in these scenarios.
AB#53297