[BUG] Panic on sysdb when calling CheckCollection #4899
Merged
Conversation
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
PR #4722 introduced a new field to the CheckCollectionsResponse, but does not populate the slice/array necessary to return the result, ending up in a null-ptr deref. This PR populates the array at top of the loop.
Contributor
|
This PR addresses a null pointer dereference issue in the CheckCollections gRPC method by ensuring the LogPosition slice is properly allocated before use. This resolves a panic introduced by PR #4722 when the LogPosition field was added but not initialized. This summary was automatically generated by @propel-code-bot |
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
eculver
approved these changes
Jun 20, 2025
sanketkedia
approved these changes
Jun 20, 2025
chroma-droid
pushed a commit
that referenced
this pull request
Jun 20, 2025
## Description of changes PR #4722 introduced a new field to the CheckCollectionsResponse, but does not populate the slice/array necessary to return the result, ending up in a null-ptr deref. This PR populates the array at top of the loop. ## Test plan CI + review ## Documentation Changes N/A
rescrv
added a commit
that referenced
this pull request
Jun 20, 2025
This PR cherry-picks the commit c3c23f3 onto rc/2025-06-20. If there are unresolved conflicts, please resolve them manually. Co-authored-by: Robert Escriva <[email protected]>
eculver
added a commit
that referenced
this pull request
Jun 25, 2025
… flexible (#4912) ## Summary This adds a test for the `CheckCollections` method. This would have caught the issue fixed in #4899. Along the way, I discovered how these tests need some love, so I started down this path by introducing a new `daotest` package that provides testing helpers for creating `Collection` values that are easier to configure. Instead of having to potentially debug all the tests that use `dao.CreateTestCollection`, I defined a "shim" that can be used as a stop-gap so that I can change the signature of this `dao.CreateTestCollection` without potentially derailing the original goal which was just to add some missing test coverage. - Improvements & Bug fixes - Added test coverage for `CheckCollections` service endpoint - New functionality - Added `daotest` package and `NewTestCollection` helper with builder/options methods for more configurability ## Test plan All the tests are passing. ## Documentation Changes N/A
Inventrohyder
pushed a commit
to Inventrohyder/chroma
that referenced
this pull request
Aug 5, 2025
## Description of changes PR chroma-core#4722 introduced a new field to the CheckCollectionsResponse, but does not populate the slice/array necessary to return the result, ending up in a null-ptr deref. This PR populates the array at top of the loop. ## Test plan CI + review ## Documentation Changes N/A
Inventrohyder
pushed a commit
to Inventrohyder/chroma
that referenced
this pull request
Aug 5, 2025
… flexible (chroma-core#4912) ## Summary This adds a test for the `CheckCollections` method. This would have caught the issue fixed in chroma-core#4899. Along the way, I discovered how these tests need some love, so I started down this path by introducing a new `daotest` package that provides testing helpers for creating `Collection` values that are easier to configure. Instead of having to potentially debug all the tests that use `dao.CreateTestCollection`, I defined a "shim" that can be used as a stop-gap so that I can change the signature of this `dao.CreateTestCollection` without potentially derailing the original goal which was just to add some missing test coverage. - Improvements & Bug fixes - Added test coverage for `CheckCollections` service endpoint - New functionality - Added `daotest` package and `NewTestCollection` helper with builder/options methods for more configurability ## Test plan All the tests are passing. ## Documentation Changes N/A
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.
Description of changes
PR #4722 introduced a new field to the CheckCollectionsResponse, but
does not populate the slice/array necessary to return the result, ending
up in a null-ptr deref. This PR populates the array at top of the loop.
Test plan
CI + review
Documentation Changes
N/A