Repository navigation
Conversation
The caching DCP feed starts before changeCache.Start sets initialSequence, so the feed is running while Start writes the field. DocChanged and processPrincipalDoc read initialSequence without the cache lock. A feed event that arrived while the database was starting raced with the write in Start. TestPushReplicationAPIUpdateDatabase hits this by reloading a database while documents are being written. Store initialSequence as an atomic.Uint64. An event that arrives before Start can still read zero and pass the startup filter, but processEntry waits for the cache lock and then drops it as a duplicate of a sequence below nextSequence. Add TestDocChangedBeforeStart, which delivers feed events between Init and Start. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
Adds coverage for DCP feed events arriving during startup and makes changeCache.initialSequence safe for concurrent reads/writes.
Changes:
- Added a new test to validate
DocChangedbehavior when events arrive betweenInitandStart. - Switched
initialSequenceto an atomic uint64 and updated reads/writes to useLoad/Store.
| File | Description |
|---|---|
| db/changecachetest/change_cache_test.go | Adds a startup-timing regression test for DocChanged/cache filtering. |
| db/change_cache.go | Makes initialSequence atomic and updates comparison/set sites accordingly. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| logsDisabled bool // If true, ignore incoming tap changes | ||
| nextSequence uint64 // Next consecutive sequence number to add. State variable for sequence buffering tracking. Should use getNextSequence() rather than accessing directly. | ||
| initialSequence uint64 // DB's current sequence at startup time. | ||
| initialSequence atomic.Uint64 // DB's current sequence at startup time. Atomic because DocChanged reads it without the lock, and the feed starts before Start sets it. |
There was a problem hiding this comment.
This is fine DocChanged may not flag as a sequence below initial sequence on startup when initial sequence is 0 (not yet set) but procesEntry duplicate checking will filter it out and by the time any goroutine acquires change cache lock this value must be set.
| var wg sync.WaitGroup | ||
| for _, event := range events { | ||
| wg.Go(func() { | ||
| changeCache.DocChanged(event, db.DocTypeDocument) | ||
| }) | ||
| } | ||
|
|
||
| require.NoError(t, changeCache.Start(initialSequence)) | ||
| wg.Wait() |
There was a problem hiding this comment.
Goroutines are needed here otherwise will deadlock. I agree though that this doesn't guarantee that the events are delivered before Start is called. But we already have testing around that handling so no need to worry about the ordering here.
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The caching DCP feed starts before
changeCache.StartsetsinitialSequence, so the feed is running whileStartwrites the field.DocChangedandprocessPrincipalDocreadinitialSequencewithout the cache lock. A feed event that arrived while the database was starting raced with the write inStart.TestPushReplicationAPIUpdateDatabase(CBG-1903) hits this by reloading a database while documents are being written.Store
initialSequenceas anatomic.Uint64. An event that arrives beforeStartcan still read zero and pass the startup filter, butprocessEntrywaits for the cache lock and then drops it as a duplicate of a sequence belownextSequence.Add
TestDocChangedBeforeStart, which delivers feed events betweenInitandStart. Without the fix, it fails under-racewith the same data race.Testing:
go test -race ./db/... ./rest/...(EE) passes.TestPushReplicationAPIUpdateDatabasefrom CBG-1903, applied on top of this change, passed 5 of 5 runs with-race.🤖 Generated with Claude Code