Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the panic in the revision cache Put path by introducing structural validation for DocumentRevision and propagating errors from Put/Upsert up through the revision cache layers. It also updates callers/tests to handle the new error-returning signatures and adjusts memory-accounting tests to reflect history bytes being counted.
Changes:
- Change
RevisionCache/collection wrappers/orchestrator/sharded cachePutandUpsertto returnerror, and update call sites to handle it. - Add
DocumentRevision.IsValid()validation and return a redactable error instead of panicking when required fields are missing. - Update revision/delta cache tests to use valid history/body inputs and to assert the revised memory byte accounting; add a new test for invalid revision validation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rest/replicatortest/replicator_test.go | Updates test to assert Upsert returns no error after signature change. |
| db/revision_cache_test.go | Updates tests for new Put/Upsert error returns and revised memory accounting; adds invalid-revision test. |
| db/revision_cache_orchestrator.go | Propagates Put/Upsert errors and only triggers eviction on success. |
| db/revision_cache_lru.go | Removes panic, adds IsValid and returns validation errors from Put/Upsert. |
| db/revision_cache_interface.go | Updates RevisionCache interface and collection wrapper methods to return errors. |
| db/revision_cache_bypass.go | Updates bypass implementation to match new interface (returns nil error). |
| db/delta_cache_test.go | Adjusts delta cache test inputs/expectations to satisfy new validation and byte accounting. |
| db/crud.go | Handles revision cache insertion errors by logging (without failing the write). |
bbrks
requested changes
May 7, 2026
Member
bbrks
left a comment
There was a problem hiding this comment.
Looks mostly OK - think we should have better error reporting if docs end up in this invalid state
bbrks
approved these changes
May 11, 2026
RIT3shSapata
added a commit
that referenced
this pull request
May 15, 2026
Squashed commit of the following: commit 8d1f7e9 Author: Ben Brooks <ben.brooks@couchbase.com> Date: Thu May 14 15:15:55 2026 +0100 CBG-5386: Grafana generator: Stamp `"version": 1` (#8274) commit f3e5570 Author: Gregory Newman-Smith <109068393+gregns1@users.noreply.github.com> Date: Thu May 14 12:46:37 2026 +0100 CBG-5381: fix resync regenerate sequences set sync info no-op (#8267) commit f277042 Author: Gregory Newman-Smith <109068393+gregns1@users.noreply.github.com> Date: Thu May 14 12:44:08 2026 +0100 CBG-5249: collect value of GOMEMLIMIT in sgcollect (#8269) commit 68fbc4c Author: Tor Colvin <tor.colvin@couchbase.com> Date: Wed May 13 20:07:59 2026 -0400 Move a db scoped function from DatabaseCollection (#8248) commit 8173a32 Author: Ben Brooks <ben.brooks@couchbase.com> Date: Wed May 13 00:21:18 2026 +0100 Improve agent instructions (#8260) commit d440374 Author: Tor Colvin <tor.colvin@couchbase.com> Date: Tue May 12 12:04:17 2026 -0400 CBG-5376 skip test temporarily (#8265) commit d17c077 Author: Ben Brooks <ben.brooks@couchbase.com> Date: Tue May 12 14:30:16 2026 +0100 CBG-5298: Cleanup for GetUser (#8259) commit a3952d3 Author: Ben Brooks <ben.brooks@couchbase.com> Date: Tue May 12 11:23:32 2026 +0100 Bump golang.org/x modules (#8255) commit fbd2d74 Author: Gregory Newman-Smith <109068393+gregns1@users.noreply.github.com> Date: Tue May 12 10:33:31 2026 +0100 CBG-5365-followup: fix escaping issue (#8256) commit 3452ce2 Author: Tor Colvin <tor.colvin@couchbase.com> Date: Mon May 11 11:45:54 2026 -0400 Fix issues from go vet: (#8251) commit f419c1e Author: Ben Brooks <ben.brooks@couchbase.com> Date: Mon May 11 16:45:40 2026 +0100 CBG-5266: Prevent downgrades across cluster compatibility versions (#8235) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> commit d09f0fd Author: Ben Brooks <ben.brooks@couchbase.com> Date: Mon May 11 16:41:09 2026 +0100 CI: upload rendered test logs as artifacts on failure (#8246) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> commit cb95e1d Author: Gregory Newman-Smith <109068393+gregns1@users.noreply.github.com> Date: Mon May 11 16:29:42 2026 +0100 CBG-5365: escape . characters for channel removal macro expansion (#8241) commit 51a86de Author: Ben Brooks <ben.brooks@couchbase.com> Date: Mon May 11 16:24:26 2026 +0100 CBG-5290: Wire up ctx into sg-bucket apis (#8240) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> commit 8ce49b3 Author: Gregory Newman-Smith <109068393+gregns1@users.noreply.github.com> Date: Mon May 11 14:44:56 2026 +0100 CBG-5260: remove panic call in rev cache Put (#8237) commit 455e037 Author: Tor Colvin <tor.colvin@couchbase.com> Date: Mon May 11 09:13:29 2026 -0400 Fix TestImportPartitionsServerless in CE mode (#8250) commit c049b7c Author: Ben Brooks <ben.brooks@couchbase.com> Date: Mon May 11 10:01:40 2026 +0100 CBG-5291: CAS-safe Update on dual MetadataStore wrapper (#8238) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> commit 6890541 Author: Tor Colvin <tor.colvin@couchbase.com> Date: Fri May 8 18:50:29 2026 -0400 CBG-4920 Fix test flake: TestActiveReplicatorHLVConflictCustom (#8247) commit 0c4ca83 Author: Tor Colvin <tor.colvin@couchbase.com> Date: Fri May 8 16:16:39 2026 -0400 CBG-5175: resync - purge checkpoints if needed (#8233) Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> commit eb3acba Author: Tor Colvin <tor.colvin@couchbase.com> Date: Fri May 8 11:10:03 2026 -0400 CBG-4496 add regression test for SetRaw/AddRaw for rosmar (#8239) Co-authored-by: Ben Brooks <ben.brooks@couchbase.com> commit 24da9f1 Author: Gregory Newman-Smith <109068393+gregns1@users.noreply.github.com> Date: Fri May 8 14:46:41 2026 +0100 CBG-5355: fix panic for on demand get when ErrImportCancelled is returned from import (#8225)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CBG-5260
Removes explicit panic call in
Puton revision cache. Adds validation on document revision and if invalid pass error up the stack forPutandUpsertmethods.Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiDependencies (if applicable)
Integration Tests