Fix replica crash by using legacy ACL path during replication #510
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
Modify AclPrefixCheck to use the new fast ACL API only for real user clients. During replication context, fall back to the legacy ACL path which has proper safety checks and prevents segmentation fault in VM_GetModuleUserFromUserName. This approach: - Preserves fast ACL checking for real users when new API is available - Uses safe legacy path during replication/AOF/internal operations - Prevents crashes while maintaining performance benefits Fixes crash introduced in commit e9f277c when ACL checking was added.
yairgott
approved these changes
Dec 1, 2025
yairgott
reviewed
Dec 2, 2025
I, Elias Tamraz <[email protected]>, hereby add my Signed-off-by to this commit: 0384de5 Address code review feedback: - Add early return for non-real users to avoid unnecessary work - Replace conditional check with assertion for better error handling Co-authored-by: yairgott Signed-off-by: Elias Tamraz <[email protected]>
yairgott
approved these changes
Dec 2, 2025
Collaborator
Author
|
@yairgott anything pending here ? |
allenss-amazon
pushed a commit
that referenced
this pull request
Dec 9, 2025
* Revise Save/Restore for true pit snapshot. (#401) * fix mem leak in unit test (#492) Signed-off-by: Miles Song <[email protected]> * Add support for checking ACL key prefix permissions using the new Valkey module API. (#479) * Add support for checking ACL key prefix permissions in Valkey module This change introduces a new function `AclValkeyCheckPermissions` that leverages Valkey's native ACL infrastructure to verify user permissions for key prefixes. It uses the new `ValkeyModule_ACLCheckKeyPrefixPermissions` API when available, falling back to the legacy `Call(ACL,...)` approach for older Valkey versions. Additionally, the build script is updated to properly detect and use the configured build tool (make or ninja) for CMake integration. Updated `.clang-format` for `PointerAlignment: Right` (with `clang-format v21` pointer is aligned **LEFT** with Google's style). * ACL permission checking: Added fast-path permission validation using ValkeyModule_ACLCheckKeyPrefixPermissions * Build system: Improved build tool detection logic in build.sh * Managed pointers: Added UniqueValkeyModuleUser for automatic ValkeyModuleUser cleanup * Header reorganization: Fixed include paths in commands.h ** Generated by CodeLite. ** Signed-off-by: Eran Ifrah <[email protected]> * Addressed PR comments. Signed-off-by: Eran Ifrah <[email protected]> --------- Signed-off-by: Eran Ifrah <[email protected]> * Fixed regression which broke Ninja builds OO (#494) * Fixed regression which broke Ninja builds OO Signed-off-by: Eran Ifrah <[email protected]> * Fix Missing OS Detection in determine_ninja Function The determine_ninja function was missing the os_name variable initialization, which is needed for the platform check. Added the local os_name assignment using uname -s to properly detect the operating system before checking for Darwin. Signed-off-by: Eran Ifrah <[email protected]> --------- Signed-off-by: Eran Ifrah <[email protected]> * Fix flaky save/restore test (#495) * Implement queue wait time aware local node preference in fanout operations (#428) * Implement queue wait time logic for local node preference in fanout operations - Add configurable low-utilization-threshold option (default 50ms) - Optimize fanout target selection to prefer local node when task wait time in queue is below threshold - Monitor both reader and writer thread pool queue wait time for utilization decisions - Maintain random selection fallback when local preference unavailable or system under high load - Extend integration tests to validate new fanout target selection behavior - Refactor thread pools to track configurable size of queue wait time samples for utilization calculation This optimization reduces network overhead and improves query latency by keeping operations local when the system has spare capacity. Signed-off-by: yulazariy <[email protected]> * fix clang format Signed-off-by: yulazariy <[email protected]> * Fix the low utilization to use the cluster map mechanism Signed-off-by: yulazariy <[email protected]> * Move all prefer local node logic to fanout.cc. minor build fixes Signed-off-by: yulazariy <[email protected]> * Fix formating Signed-off-by: yulazariy <[email protected]> * Revert new timeout function Signed-off-by: yulazariy <[email protected]> --------- Signed-off-by: yulazariy <[email protected]> * Populate fingerprint and version to IndexSchema (#480) * populate fingerprint and version to IndexSchema Signed-off-by: Miles Song <[email protected]> * remove logs Signed-off-by: Miles Song <[email protected]> * minor fix according to comments Signed-off-by: Miles Song <[email protected]> * load rdb integration tests Signed-off-by: Miles Song <[email protected]> * fix clang tidy Signed-off-by: Miles Song <[email protected]> * clean protobuf; populate fingerprint/version in MetadataManager::OnLoadingEnd Signed-off-by: Miles Song <[email protected]> * rebase Signed-off-by: Miles Song <[email protected]> * fix integration/run.sh and disable full trace Signed-off-by: Miles Song <[email protected]> * fix according to comments Signed-off-by: Miles Song <[email protected]> * fix spellcheck Signed-off-by: Miles Song <[email protected]> --------- Signed-off-by: Miles Song <[email protected]> * Fix warnings and formatting (#503) Signed-off-by: Allen Samuels <[email protected]> * Fix Override Specifier and Suppress MacOS Compiler Warning (#496) Suppress the -Wdefaulted-function-deleted warning on Apple platforms to address compiler-specific diagnostics. Also apply minor formatting improvements to CMake configuration for better readability. * src/commands/ft_dropindex.cc * CMakeLists.txt ** Generated by CodeLite. ** Signed-off-by: Eran Ifrah <[email protected]> Co-authored-by: Allen Samuels <[email protected]> * feat: set command info for ft.create (#299) * feat: set command info for ft.create Signed-off-by: proost <[email protected]> * fix: add file Signed-off-by: proost <[email protected]> * fix: wrong command arg format Signed-off-by: proost <[email protected]> * test: remove print Signed-off-by: proost <[email protected]> * test: fix broken test Signed-off-by: proost <[email protected]> * fix: wrong sub args Signed-off-by: proost <[email protected]> * style: format Signed-off-by: proost <[email protected]> * style: follow lint Signed-off-by: proost <[email protected]> * style: follow lint Signed-off-by: proost <[email protected]> --------- Signed-off-by: proost <[email protected]> Signed-off-by: Allen Samuels <[email protected]> Co-authored-by: Allen Samuels <[email protected]> * FT.SEARCH partition and consistency controls (#464) * allshards/someshards for ft.search Signed-off-by: Miles Song <[email protected]> * ft.search partition tests Signed-off-by: Miles Song <[email protected]> * ft.search partition and consistency controls Signed-off-by: Miles Song <[email protected]> * fix ft.search partition controls test Signed-off-by: Miles Song <[email protected]> * fix format Signed-off-by: Miles Song <[email protected]> * add configurable PreferConsistentResults Signed-off-by: Miles Song <[email protected]> * refactor duplicate code in server.cc Signed-off-by: Miles Song <[email protected]> * rebase; populate fingerprint/version from IndexSchema Signed-off-by: Miles Song <[email protected]> * remove optional in proto; always check index schema consistency Signed-off-by: Miles Song <[email protected]> --------- Signed-off-by: Miles Song <[email protected]> * FT.INFO partition and consistency controls (#469) * move fingeprint version check to server.cc Signed-off-by: Miles Song <[email protected]> * ft.info partition and consistency controls Signed-off-by: Miles Song <[email protected]> * remove unused logs Signed-off-by: Miles Song <[email protected]> * add lock for onresponse Signed-off-by: Miles Song <[email protected]> * fix according to comments Signed-off-by: Miles Song <[email protected]> * rebase from main; populate fingerprint/version from IndexSchema in server.cc Signed-off-by: Miles Song <[email protected]> * fix fanout operations Signed-off-by: Miles Song <[email protected]> * rebase; add slot consistency check in ft.info Signed-off-by: Miles Song <[email protected]> * fix integration tests Signed-off-by: Miles Song <[email protected]> --------- Signed-off-by: Miles Song <[email protected]> * Limit and Nocontent support for non vector queries (#522) * Limit and Nocontent support for non vector Signed-off-by: Karthik Subbarao <[email protected]> * use ReplyAvailNeighbors Signed-off-by: Karthik Subbarao <[email protected]> * Use ReplyAvailNeighbors Signed-off-by: Karthik Subbarao <[email protected]> * fmt Signed-off-by: Karthik Subbarao <[email protected]> * Add integ testing for non vector queries with LIMIT and NOCONTENT Signed-off-by: Karthik Subbarao <[email protected]> * include tests for CME Signed-off-by: Karthik Subbarao <[email protected]> * add limit Signed-off-by: Karthik Subbarao <[email protected]> --------- Signed-off-by: Karthik Subbarao <[email protected]> * Fix compatibility test for ft.search use-cases (not included yet). Add documentation * Fix test Signed-off-by: Allen Samuels <[email protected]> * Add documentation Signed-off-by: Allen Samuels <[email protected]> --------- Signed-off-by: Allen Samuels <[email protected]> * Full support for DB numbers other than zero in cluster mode. (#410) * Initial IndexName encode/decode Signed-off-by: Allen Samuels <[email protected]> * working now Signed-off-by: Allen Samuels <[email protected]> * format Signed-off-by: Allen Samuels <[email protected]> * formatting, remove unneded logs Signed-off-by: Allen Samuels <[email protected]> * spelling Signed-off-by: Allen Samuels <[email protected]> * formatting Signed-off-by: Allen Samuels <[email protected]> * Update for aggregation Signed-off-by: Allen Samuels <[email protected]> * Rewrite per feedback, move encoding into metadata_manager Signed-off-by: Allen Samuels <[email protected]> * fix merge errors Signed-off-by: Allen Samuels <[email protected]> * Revise per-object Encoding Version Signed-off-by: Allen Samuels <[email protected]> * fix integration test Signed-off-by: Allen Samuels <[email protected]> * Enhance metadata tests Signed-off-by: Allen Samuels <[email protected]> * Update semantic versioning Signed-off-by: Allen Samuels <[email protected]> * Implement schema-level versioning Signed-off-by: Allen Samuels <[email protected]> * include omitted test file Signed-off-by: Allen Samuels <[email protected]> * fix valkey version post GA Signed-off-by: Allen Samuels <[email protected]> * remove old drop option Signed-off-by: Allen Samuels <[email protected]> * delete check Signed-off-by: Allen Samuels <[email protected]> * remerge Signed-off-by: Allen Samuels <[email protected]> * fix integration test Signed-off-by: Allen Samuels <[email protected]> * redo Signed-off-by: Allen Samuels <[email protected]> * more Signed-off-by: Allen Samuels <[email protected]> * better Signed-off-by: Allen Samuels <[email protected]> * more Signed-off-by: Allen Samuels <[email protected]> * Fixes for non-routable Signed-off-by: Allen Samuels <[email protected]> * fix unit tests Signed-off-by: Allen Samuels <[email protected]> * Fix last test mismatch Signed-off-by: Allen Samuels <[email protected]> * Introduce ObjName Signed-off-by: Allen Samuels <[email protected]> * Revert "feat: set command info for ft.create (#299)" This reverts commit c50f841. Signed-off-by: Allen Samuels <[email protected]> * cleanup Signed-off-by: Allen Samuels <[email protected]> * Cleanup 1 Signed-off-by: Allen Samuels <[email protected]> * cleanup 2 Signed-off-by: Allen Samuels <[email protected]> * cleanup 3 Signed-off-by: Allen Samuels <[email protected]> --------- Signed-off-by: Allen Samuels <[email protected]> * Fix replica crash by using legacy ACL path during replication (#510) * Fix replica crash by using legacy ACL path during replication Modify AclPrefixCheck to use the new fast ACL API only for real user clients. During replication context, fall back to the legacy ACL path which has proper safety checks and prevents segmentation fault in VM_GetModuleUserFromUserName. This approach: - Preserves fast ACL checking for real users when new API is available - Uses safe legacy path during replication/AOF/internal operations - Prevents crashes while maintaining performance benefits Fixes crash introduced in commit e9f277c when ACL checking was added. * DCO Remediation Commit for Elias Tamraz <[email protected]> I, Elias Tamraz <[email protected]>, hereby add my Signed-off-by to this commit: 0384de5 Address code review feedback: - Add early return for non-real users to avoid unnecessary work - Replace conditional check with assertion for better error handling Co-authored-by: yairgott Signed-off-by: Elias Tamraz <[email protected]> --------- Signed-off-by: Elias Tamraz <[email protected]> * use negate flag parser in ft.info parser (#524) Signed-off-by: Miles Song <[email protected]> * fix text index Signed-off-by: Miles Song <[email protected]> * fix format Signed-off-by: Miles Song <[email protected]> --------- Signed-off-by: Miles Song <[email protected]> Signed-off-by: Eran Ifrah <[email protected]> Signed-off-by: yulazariy <[email protected]> Signed-off-by: Allen Samuels <[email protected]> Signed-off-by: proost <[email protected]> Signed-off-by: Karthik Subbarao <[email protected]> Signed-off-by: Elias Tamraz <[email protected]> Co-authored-by: Allen Samuels <[email protected]> Co-authored-by: eifrah-aws <[email protected]> Co-authored-by: yulazariy <[email protected]> Co-authored-by: Hyeonho Kim <[email protected]> Co-authored-by: KarthikSubbarao <[email protected]> Co-authored-by: Elias Tamraz <[email protected]> DCO Remediation Commit for Allen Samuels <[email protected]> I, Allen Samuels <[email protected]>, hereby add my Signed-off-by to this commit: c2c1e7b I, Allen Samuels <[email protected]>, hereby add my Signed-off-by to this commit: be8b236 Signed-off-by: Allen Samuels <[email protected]> Third-Party DCO Remediation Commit for eifrah-aws <[email protected]> On behalf of eifrah-aws <[email protected]>, I, Allen Samuels <[email protected]>, hereby add my Signed-off-by to this commit: e9f277c On behalf of eifrah-aws <[email protected]>, I, Allen Samuels <[email protected]>, hereby add my Signed-off-by to this commit: cdfe0c4 On behalf of eifrah-aws <[email protected]>, I, Allen Samuels <[email protected]>, hereby add my Signed-off-by to this commit: de64571 Signed-off-by: Allen Samuels <[email protected]> Third-Party DCO Remediation Commit for Elias Tamraz <[email protected]> On behalf of Elias Tamraz <[email protected]>, I, Allen Samuels <[email protected]>, hereby add my Signed-off-by to this commit: 990efa0 Signed-off-by: Allen Samuels <[email protected]> Third-Party DCO Remediation Commit for Hyeonho Kim <[email protected]> On behalf of Hyeonho Kim <[email protected]>, I, Allen Samuels <[email protected]>, hereby add my Signed-off-by to this commit: c50f841 Signed-off-by: Allen Samuels <[email protected]> Third-Party DCO Remediation Commit for KarthikSubbarao <[email protected]> On behalf of KarthikSubbarao <[email protected]>, I, Allen Samuels <[email protected]>, hereby add my Signed-off-by to this commit: 6706ea2 Signed-off-by: Allen Samuels <[email protected]> Third-Party DCO Remediation Commit for Miles Song <[email protected]> On behalf of Miles Song <[email protected]>, I, Allen Samuels <[email protected]>, hereby add my Signed-off-by to this commit: d90a358 On behalf of Miles Song <[email protected]>, I, Allen Samuels <[email protected]>, hereby add my Signed-off-by to this commit: 57e5dfd On behalf of Miles Song <[email protected]>, I, Allen Samuels <[email protected]>, hereby add my Signed-off-by to this commit: 0026dfc On behalf of Miles Song <[email protected]>, I, Allen Samuels <[email protected]>, hereby add my Signed-off-by to this commit: b81574a On behalf of Miles Song <[email protected]>, I, Allen Samuels <[email protected]>, hereby add my Signed-off-by to this commit: 31d37d9 Signed-off-by: Allen Samuels <[email protected]>
AlexFilipImproving
pushed a commit
to Bit-Quill/valkey-search
that referenced
this pull request
Dec 10, 2025
…-io#510) * Fix replica crash by using legacy ACL path during replication Modify AclPrefixCheck to use the new fast ACL API only for real user clients. During replication context, fall back to the legacy ACL path which has proper safety checks and prevents segmentation fault in VM_GetModuleUserFromUserName. This approach: - Preserves fast ACL checking for real users when new API is available - Uses safe legacy path during replication/AOF/internal operations - Prevents crashes while maintaining performance benefits Fixes crash introduced in commit e9f277c when ACL checking was added. * DCO Remediation Commit for Elias Tamraz <[email protected]> I, Elias Tamraz <[email protected]>, hereby add my Signed-off-by to this commit: 0384de5 Address code review feedback: - Add early return for non-real users to avoid unnecessary work - Replace conditional check with assertion for better error handling Co-authored-by: yairgott Signed-off-by: Elias Tamraz <[email protected]> --------- Signed-off-by: Elias Tamraz <[email protected]>
AlexFilipImproving
pushed a commit
to Bit-Quill/valkey-search
that referenced
this pull request
Dec 10, 2025
…-io#510) * Fix replica crash by using legacy ACL path during replication Modify AclPrefixCheck to use the new fast ACL API only for real user clients. During replication context, fall back to the legacy ACL path which has proper safety checks and prevents segmentation fault in VM_GetModuleUserFromUserName. This approach: - Preserves fast ACL checking for real users when new API is available - Uses safe legacy path during replication/AOF/internal operations - Prevents crashes while maintaining performance benefits Fixes crash introduced in commit e9f277c when ACL checking was added. * DCO Remediation Commit for Elias Tamraz <[email protected]> I, Elias Tamraz <[email protected]>, hereby add my Signed-off-by to this commit: 0384de5 Address code review feedback: - Add early return for non-real users to avoid unnecessary work - Replace conditional check with assertion for better error handling Co-authored-by: yairgott Signed-off-by: Elias Tamraz <[email protected]> --------- Signed-off-by: Elias Tamraz <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Resolves #509