Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes CBG-5106, where delta-sync rev messages sent to a legacy (pre-HLV) client could end up with an empty/incorrect history property, breaking conflict detection on the receiver. The fix centralizes the previously duplicated history-construction logic across sendRevision, sendDelta, and sendRevAsDelta (redacted path), and propagates the remoteIsLegacyRev flag through sendRevAsDelta into sendDelta so the delta path can produce the same history as the full-revision path.
Changes:
- Introduce
buildRevHistory/revHistoryInputto centralize rev-message history construction for the 5 local/remote legacy-vs-HLV scenarios, with extensive scenario documentation. - Plumb
remoteIsLegacyRevthroughsendRevAsDelta→sendDelta(and fallbacksendRevisioncalls) so deltas to legacy clients include[hlvHistory, revID, revTreeHistory...]. - Add two new lower-level BLIP tests for the delta + legacy-client case and unskip the previously skipped
TestActiveReplicatorDeltaSyncWhenBothSidesLegacy.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| db/blip_sync_context.go | Adds buildRevHistory helper with scenario docs; sendDelta now takes remoteIsLegacyRev and uses the helper; sendRevision refactored to use the helper. |
| db/blip_handler.go | sendRevAsDelta takes remoteIsLegacyRev, propagates it to fallback sendRevision and sendDelta calls, and uses buildRevHistory for the redacted-rev path. |
| rest/blip_legacy_revid_test.go | Adds two tests verifying delta history sent to legacy clients (with and without HLV pv history). |
| rest/replicatortest/replicator_test_legacy_rev_test.go | Removes t.Skip from TestActiveReplicatorDeltaSyncWhenBothSidesLegacy now that the fix lands. |
torcolvin
left a comment
There was a problem hiding this comment.
I think this code looks good but I would feel much better about the tests if they didn't override the handlers and used the standard BlipTestClientRunner. Is there a reason taht they aren't using this?
|
Do we need an extra tests for ISGR? |
gregns1
left a comment
There was a problem hiding this comment.
No extra test needed for ISGR, ISGR already has delta sync tests but the original bug wasn't found as rev trees are always sent in ISGR case in the rev tree property.
CBG-5106
QE tests now show history being sent correctly for both test cases in the linked ticket:
Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiIntegration Tests