Conversation
When ActiveOnly is set, the N1QL query will filtered tombstoned documents but not documents which have undergone a channel filter. To make sure this works in cases of mixed active/inactive documents and channel backfill: - Filter non active entries from getChangesInChannelFromQuery that are not caught by N1QL (channel removals) - Fill the active entries from the cache, which could involve parsing a greater number of cached entries than limit has
There was a problem hiding this comment.
Pull request overview
Fixes an edge case in the channel cache changes retrieval path where active_only=true combined with limit could lead to returning fewer active changes than requested, especially when non-active entries (e.g., channel removals) are interleaved with active documents.
Changes:
- Filter non-active (removal/deleted) entries during
getChangesInChannelFromQuerywhenactiveOnlyis set, so limit calculations operate on active entries only. - Adjust
singleChannelCacheImpl.GetChangesto scan cached entries and skip non-active ones when trying to fill up toLimitinActiveOnlymode. - Add a unit test covering
ActiveOnly+Limitbehavior with a small channel cache length.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
db/channel_cache_test.go |
Adds a regression test for active_only=true with limit when cache contains mixed active/removal entries. |
db/channel_cache_single.go |
Updates cache+query merge logic to avoid limit being “consumed” by non-active cached entries in ActiveOnly mode. |
db/changes_view.go |
Filters out non-active query rows in activeOnly mode and updates limit loop logic accordingly. |
| if !options.ActiveOnly || len(resultFromQuery)+len(resultFromCache) <= options.Limit { | ||
| result = append(result, resultFromCache[0:n]...) | ||
| } else { | ||
| totalEntries := len(result) | ||
| for _, entry := range resultFromCache { | ||
| if totalEntries >= options.Limit { | ||
| break | ||
| } | ||
| if !entry.IsActive() { | ||
| continue | ||
| } | ||
| result = append(result, entry) | ||
| totalEntries++ | ||
| } |
There was a problem hiding this comment.
When ActiveOnly is true and Limit is 0 (meaning “no limit”), the new branch will take the else path and immediately break because totalEntries >= options.Limit (0). That prevents any cached entries from being appended, so GetChanges can incorrectly return only the query results and drop newer cached active entries.
Suggestion: treat Limit==0 as unlimited in this logic (e.g., include it in the fast-path append, or make the loop’s break condition conditional on options.Limit > 0). Also consider always filtering out non-active cache entries when ActiveOnly is true, even when unlimited.
| // If active-only, loop until either retrieve (limit) active entries, or reach endSeq. | ||
| if activeOnly { | ||
| // If we've reached limit, we're done | ||
| if activeEntryCount >= limit || limit == 0 { | ||
| if len(entries) >= limit || limit == 0 { | ||
| break |
There was a problem hiding this comment.
In activeOnly mode, entries accumulates across loop iterations, but each call to QueryChannels still uses the original limit. After filtering out non-active rows, a later iteration can append more than the remaining needed entries, resulting in len(entries) > limit and violating the requested limit. Consider querying with remaining := limit - len(entries) (when limit > 0) and/or truncating entries to limit before returning.
CBG-5262 fix missing changes with active_only=true and limit=
When ActiveOnly is set, the N1QL query will filtered tombstoned documents but not documents which have undergone a channel filter. In this case, active only and limit were compatible. In the case of a channel removal, the limit
To make sure this works in cases of mixed active/inactive documents and channel backfill:
When
activeOnlyis set, that doesn't populate channel cache, so it isn't a problem to filter the entries forgetChangesInChannelFromQuery.GetChanges?Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiIntegration Tests