enhance: bump milvus-storage commit with new manifest and FFI interface #47085
Conversation
Related to milvus-io#44956 Update milvus-storage dependency to use new Manifest structure where column groups are accessed via manifest->columnGroups(). Adapt to renamed FFI functions (loon_ prefix) and updated Transaction API. Signed-off-by: Congqi Xia <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: congqixia The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
sre-ci-robot
added
approved
size/L
Summary of ChangesHello @congqixia, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on upgrading the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
mergify
bot
added
dco-passed
|
[ci-v2-notice] To rerun ci-v2 checks, comment with:
If you have any questions or requests, please contact @zhikunyao. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the milvus-storage dependency and adapts the codebase to its new API. The changes include using a new Manifest structure, renamed FFI functions with a loon_ prefix, and an updated Transaction API. The changes are widespread across both C++ and Go code.
While most of the changes correctly adapt to the new API, I've found some critical issues in the Go FFI wrapper code related to memory and resource lifetime management. Specifically, there's a use-after-free bug in packed_reader_ffi.go that could lead to crashes, and potential memory leaks in both packed_reader_ffi.go and packed_writer_ffi.go. These issues need to be addressed to ensure stability.
| func GetManifestHandle(manifestPath string, storageConfig *indexpb.StorageConfig) (loonManifestHandle *C.LoonManifest, err error) { | ||
| var cManifestHandle *C.LoonManifest | ||
| basePath, version, err := UnmarshalManfestPath(manifestPath) | ||
| if err != nil { | ||
| return cColumnGroups, err | ||
| return cManifestHandle, err | ||
| } | ||
| log.Info("GetManifest", zap.String("manifestPath", manifestPath), zap.String("basePath", basePath), zap.Int64("version", version)) | ||
|
|
||
| cProperties, err := MakePropertiesFromStorageConfig(storageConfig, nil) | ||
| if err != nil { | ||
| return cColumnGroups, err | ||
| return cManifestHandle, err | ||
| } | ||
| cBasePath := C.CString(basePath) | ||
| defer C.free(unsafe.Pointer(cBasePath)) | ||
|
|
||
| result := C.get_column_groups_by_version(cBasePath, cProperties, C.int64_t(version), &cColumnGroups) | ||
| err = HandleFFIResult(result) | ||
| var cTransactionHandle C.LoonTransactionHandle | ||
| result := C.loon_transaction_begin(cBasePath, cProperties, C.int64_t(version), 1, &cTransactionHandle) | ||
| err = HandleLoonFFIResult(result) | ||
| if err != nil { | ||
| return cColumnGroups, err | ||
| return cManifestHandle, err | ||
| } | ||
| defer C.loon_transaction_destroy(cTransactionHandle) | ||
|
|
||
| return cColumnGroups, nil | ||
| result = C.loon_transaction_get_manifest(cTransactionHandle, &cManifestHandle) | ||
| err = HandleLoonFFIResult(result) | ||
| if err != nil { | ||
| return cManifestHandle, err | ||
| } | ||
|
|
||
| return cManifestHandle, nil | ||
| } |
There was a problem hiding this comment.
This function has two significant resource lifetime management issues that will likely lead to a crash and a memory leak.
-
Use-after-free on
cManifestHandle: Thedefer C.loon_transaction_destroy(cTransactionHandle)at line 208 will be executed whenGetManifestHandlereturns. The returnedcManifestHandleis obtained fromcTransactionHandle. It's highly likely that destroying the transaction handle invalidates the manifest handle, making it a dangling pointer. The callerNewFFIPackedReaderwill then use this dangling pointer, leading to a use-after-free, which can cause crashes or unpredictable behavior. -
Memory leak of
cProperties: ThecPropertiesobject is created byMakePropertiesFromStorageConfigat line 195. It is not freed anywhere in this function. This will cause a memory leak on every call. There should be adefer C.loon_properties_free(cProperties)call.
To fix this, you should probably refactor this function and its caller. GetManifestHandle should not destroy the transaction handle. The caller should be responsible for its lifetime. For example, GetManifestHandle could return the transaction handle as well, and the caller would defer its destruction after it's done using the manifest handle. The properties object should also be freed within this function after it's no longer needed.
| func (pw *FFIPackedWriter) Close() (string, error) { | ||
| var cColumnGroups C.ColumnGroupsHandle | ||
| var cColumnGroups *C.LoonColumnGroups | ||
|
|
||
| result := C.writer_close(pw.cWriterHandle, nil, nil, 0, &cColumnGroups) | ||
| if err := HandleFFIResult(result); err != nil { | ||
| result := C.loon_writer_close(pw.cWriterHandle, nil, nil, 0, &cColumnGroups) | ||
| if err := HandleLoonFFIResult(result); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| cBasePath := C.CString(pw.basePath) | ||
| defer C.free(unsafe.Pointer(cBasePath)) | ||
| var transationHandle C.TransactionHandle | ||
| var transationHandle C.LoonTransactionHandle | ||
|
|
||
| result = C.transaction_begin(cBasePath, pw.cProperties, &transationHandle, C.int64_t(pw.baseVersion)) | ||
| if err := HandleFFIResult(result); err != nil { | ||
| result = C.loon_transaction_begin(cBasePath, pw.cProperties, C.int64_t(pw.baseVersion), 1, &transationHandle) | ||
| if err := HandleLoonFFIResult(result); err != nil { | ||
| return "", err | ||
| } | ||
| defer C.transaction_destroy(transationHandle) | ||
| defer C.loon_transaction_destroy(transationHandle) | ||
|
|
||
| // #define LOON_TRANSACTION_UPDATE_ADDFILES 0 | ||
| // #define LOON_TRANSACTION_UPDATE_ADDFEILD 1 | ||
| // #define LOON_TRANSACTION_UPDATE_MAX 2 | ||
|
|
||
| // #define LOON_TRANSACTION_RESOLVE_FAIL 0 | ||
| // #define LOON_TRANSACTION_RESOLVE_MERGE 1 | ||
| // #define LOON_TRANSACTION_RESOLVE_MAX 2 | ||
| result = C.loon_transaction_append_files(transationHandle, cColumnGroups) | ||
| if err := HandleLoonFFIResult(result); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| var commitResult C.TransactionCommitResult | ||
| result = C.transaction_commit(transationHandle, C.int16_t(0), C.int16_t(0), cColumnGroups, &commitResult) | ||
| if err := HandleFFIResult(result); err != nil { | ||
| var cCommitVersion C.int64_t | ||
| result = C.loon_transaction_commit(transationHandle, &cCommitVersion) | ||
| if err := HandleLoonFFIResult(result); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| log.Info("FFI writer closed", zap.Int64("version", int64(commitResult.committed_version))) | ||
| log.Info("FFI writer closed", zap.Int64("version", int64(cCommitVersion))) | ||
|
|
||
| defer C.properties_free(pw.cProperties) | ||
| return MarshalManifestPath(pw.basePath, int64(commitResult.committed_version)), nil | ||
| defer C.loon_properties_free(pw.cProperties) | ||
| return MarshalManifestPath(pw.basePath, int64(cCommitVersion)), nil | ||
| } |
There was a problem hiding this comment.
There appears to be a potential memory leak for cColumnGroups within this Close function.
The variable cColumnGroups is populated by C.loon_writer_close at line 171. It's a pointer to C.LoonColumnGroups, which is likely allocated on the heap within the C++ FFI function. However, there is no corresponding call to free this memory within the Close function. If loon_transaction_append_files does not take ownership and free the memory, this will result in a memory leak every time Close is called.
Please verify the ownership semantics of cColumnGroups and add a call to free it if necessary, likely via a C.loon_column_groups_free(cColumnGroups) function if one exists.
2 participants
Related to #44956
Update milvus-storage dependency to use new Manifest structure where column groups are accessed via manifest->columnGroups(). Adapt to renamed FFI functions (loon_ prefix) and updated Transaction API.