CBG-5429: Fix SequenceID String() and Before() for LowSeq/TriggeredBy corner cases #8423
CBG-5429: Fix SequenceID String() and Before() for LowSeq/TriggeredBy corner cases #8423RIT3shSapata wants to merge 4 commits into
Conversation
Fixes String() and Before() for corner cases related to backfill boundaries (a.TriggeredBy = b.Seq) and LowSeq/TriggeredBy interactions.
…ompound sequences.
There was a problem hiding this comment.
Pull request overview
This PR fixes ordering/serialization edge cases for db.SequenceID used by the _changes feed, specifically around compound sequences that include both LowSeq (skipped-sequence mode) and TriggeredBy (channel-grant backfill). It also adds an integration test intended to prevent regressions in multi-channel backfill behavior.
Changes:
- Fix
SequenceID.String()to preserveLowSeqforLowSeq:TriggeredBy:Seqcases whereLowSeq > Seq. - Rewrite
SequenceID.Before()to correctly order sequences whenLowSeqand/orTriggeredByare present. - Add a
_changesintegration test scenario combining skipped sequences (LowSeq mode) with a mid-stream channel grant (TriggeredBy backfill).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
db/sequence_id.go |
Adjusts string formatting and comparison semantics for compound SequenceID corner cases affecting _changes ordering and continuation. |
db/sequence_id_test.go |
Extends unit coverage for String() and ordering with LowSeq+TriggeredBy combinations. |
rest/changestest/changes_api_test.go |
Adds an integration test meant to cover LowSeq+TriggeredBy behavior during changes backfill. |
| if s.LowSeq > 0 { | ||
| if s.TriggeredBy > 0 { | ||
| return fmt.Sprintf("%d:%d:%d", s.LowSeq, s.TriggeredBy, s.Seq) | ||
| } else { |
| // With seq 7 now in the cache, the changes feed should deliver it via the late-sequence | ||
| // path. The user still has no DEF access, but the LowSeq advancement itself produces a | ||
| // result (last_seq moves forward). The last_seq from this response is used as the since | ||
| // value for the final request below. | ||
| changes = rt.PostChanges("/{{.keyspace}}/_changes", changesJSON, "sg-user") | ||
| require.Len(t, changes.Results, 1) | ||
|
|
There was a problem hiding this comment.
false, you will get sequence 9 again here in this scenario when stable sequence moves on
| // Use the last_seq from the post-seq7 response as the since value. This since has LowSeq | ||
| // set, and after the DEF grant the changes feed will add TriggeredBy, producing a compound | ||
| // since value {LowSeq: L, TriggeredBy: 10, Seq: S} — the CBG-5429 corner case. | ||
| changesJSON = fmt.Sprintf(`{"since":"%s"}`, changes.Last_Seq.String()) | ||
|
|
||
| // Expect 4 results: the user update (seq 10) plus the 3 DEF backfill docs (seqs 3, 4, 7). | ||
| // If Before() or String() are broken for the compound since value, backfill docs will be | ||
| // missed and this assertion will fail. | ||
| changes = rt.PostChanges("/{{.keyspace}}/_changes", changesJSON, "sg-user") |
There was a problem hiding this comment.
I think this may be worth doing
gregns1
left a comment
gregns1
left a comment
There was a problem hiding this comment.
I think we have issues with these changes with interrupted/oneshot feeds. I don't think this is effected for continuous feeds but will be effected for continuous feeds on the first iteration of the feed. So lower blast radius.
Scenario 1 — continuing backfill skips a doc
| Seq | Channel | Note |
|---|---|---|
| 1 | (user1) | access: ABC |
| 2,3,4 | DEF | backfill targets |
| 5 | ABC | |
| 6 | — | gap (skipped) |
| 7 | ABC |
GET _changesas user1 →last_seq = 5::7.- Grant user1 access to
ABC, DEF→ lands at seq 10. POST _changeswithsince=5::7&limit=2(cuts the backfill short) →last_seq = 5:10:3.
Old code:LowSeq(5)is not< Seq(3), so it emitted10:3.- Seq 6 arrives; the feed's
lowSequencemoves to 7. POST _changeswithsince=5:10:3(no/high limit) → returns only the user doc,last_seq = 7::10.
Result: Seq 4 is not delivered in the feed. Line 850 in changes.go will see that since low seq is != 0 but since low seq != low seq on feed (it’s moved to 7 now) and this does not set since low seq to 0. This stays as 5 here. Then in changesFeed SafeSequence() calculates safe seq to be low seq, in this case 5 and this leads to sequence 4 above getting missed on the feed.
Scenario 2 — grant's fresh backfill is suppressed
| Seq | Channel | Note |
|---|---|---|
| 1 | (user1) | access: ABC |
| 2 | DEF | backfill target |
| 3 | GHI | backfill target |
| 4 | (grant) | user1 → ABC, DEF |
| 5 | (grant) | user1 → ABC, DEF, GHI |
| 6,7 | ABC | |
| 8 | — | gap (skipped) |
| 9 | ABC |
POST _changeswithlimit=1(cuts the backfill short) →last_seq = 7:4:2.- Seq 8 arrives; the feed's
lowSequencemoves off 7. POST _changeswithsince=7:4:2.
Result: GHI's backfill doc (seq 3) is not delivered in feed. On this first iteration isNewChannel is false (changedChannels is only populated on later iterations of a waiting feed), so the decision to backfill falls to backfillRequired at changes.go:929
So seqAdded at for GHI will be seq 5:
So seqAddedAt > 1 -> true
But 7:4:2.Before(5) -> false
backfillRequired is false
I think overall it may be worth discussing as a team this PR in sync up.
| // With seq 7 now in the cache, the changes feed should deliver it via the late-sequence | ||
| // path. The user still has no DEF access, but the LowSeq advancement itself produces a | ||
| // result (last_seq moves forward). The last_seq from this response is used as the since | ||
| // value for the final request below. | ||
| changes = rt.PostChanges("/{{.keyspace}}/_changes", changesJSON, "sg-user") | ||
| require.Len(t, changes.Results, 1) | ||
|
|
There was a problem hiding this comment.
false, you will get sequence 9 again here in this scenario when stable sequence moves on
| // Use the last_seq from the post-seq7 response as the since value. This since has LowSeq | ||
| // set, and after the DEF grant the changes feed will add TriggeredBy, producing a compound | ||
| // since value {LowSeq: L, TriggeredBy: 10, Seq: S} — the CBG-5429 corner case. | ||
| changesJSON = fmt.Sprintf(`{"since":"%s"}`, changes.Last_Seq.String()) | ||
|
|
||
| // Expect 4 results: the user update (seq 10) plus the 3 DEF backfill docs (seqs 3, 4, 7). | ||
| // If Before() or String() are broken for the compound since value, backfill docs will be | ||
| // missed and this assertion will fail. | ||
| changes = rt.PostChanges("/{{.keyspace}}/_changes", changesJSON, "sg-user") |
There was a problem hiding this comment.
I think this may be worth doing
CBG-5429
SequenceID.String()andSequenceID.Before()for corner cases involving both LowSeq and TriggeredBy, and add a changes feed integration test to verify no regression in backfill behaviour.String()fix: the old implementation only emitted the LowSeq prefix when LowSeq < Seq. When LowSeq > Seq (which occurs during a backfill-in-progress with active skipped sequences), the LowSeq field was silently dropped, causing the client to receive an incorrect last_seq. On the next request the changes feed would resume from the wrong position, potentially missing backfill documents.Before()fix — the old implementation routed all comparisons through SafeSequence() (which returns LowSeq when non-zero), ignoring the Seq field entirely when LowSeq is set. This produced wrong ordering for cases such as:Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiDependencies (if applicable)
Integration Tests