Revert "Update deps/libvalkey to version 0.5.0 (#3697)" on 9.1#3755
Revert "Update deps/libvalkey to version 0.5.0 (#3697)" on 9.1#3755sarthakaggarwal97 wants to merge 1 commit into
Conversation
This reverts commit 82c386d. The libvalkey 0.5.0 update breaks valkey-sentinel on 9.1 with a reproducible SIGSEGV in the cron path: dictResizeWithOptionalCheck dictExpandIfNeeded dictFindPositionForInsert dictAddRaw dictReplace valkeyAsyncAppendCmdLen (libvalkey) valkeyAsyncCommand (libvalkey) sentinelReconnectInstance (sentinel.c) sentinelHandleValkeyInstance sentinelTimer serverCron The crash reproduces on every PR targeting 9.1 (verified on PR valkey-io#3733 and the unrelated 9.1-release-version PR) and across every test job that runs the sentinel suite (jemalloc, libc-malloc, tls-module, sanitizers, almalinux/centos/fedora variants, alpine, macOS-sentinel, arm, lttng, fortify, no-malloc-usable-size, reply-schemas-validator). It does not reproduce on unstable. Root cause: libvalkey 0.5.0 includes upstream commit "e4b548b32 Remove support for external dict while still supporting external sds (#302)". After this libvalkey commit, libvalkey ships its own bundled dict.c/dict.h and is no longer designed to interoperate with an externally provided Valkey dict implementation. However, Valkey 9.1's deps/Makefile still passes DICT_INCLUDE_DIR=../../src/ when building libvalkey, so libvalkey's async.c is compiled against Valkey 9.1's legacy src/dict.h while calling Valkey 9.1's legacy dict.c symbols (dictReplace, dictAddRaw, dictResizeWithOptionalCheck). The struct dict layouts assumed by the two sides do not match, leading to the SIGSEGV during sentinel's first reconnect attempt. The same libvalkey 0.5.0 update works on unstable because PR valkey-io#3366 ("Replace dict with thin wrapper around hashtable") replaced Valkey's legacy dict with a header-only hashtable wrapper. PR valkey-io#3366 was not backported to 9.1. Reverting libvalkey back to 0.4.0 restores 9.1 sentinel functionality while we land a proper forward fix (either backport the build-system hunks of libvalkey #302 so libvalkey 0.5.0 uses its own bundled dict, or apply a 9.1-side adaptation). Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Closing this PR in favor of #3752 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 9.1 #3755 +/- ##
==========================================
- Coverage 76.53% 76.49% -0.04%
==========================================
Files 163 163
Lines 80991 80991
==========================================
- Hits 61987 61958 -29
- Misses 19004 19033 +29 🚀 New features to boost your workflow:
|
Summary
Revert 82c386d Update deps/libvalkey to version 0.5.0 (#3697) on the
9.1branch.The libvalkey 0.5.0 update breaks
valkey-sentinelon9.1with a reproducibleSIGSEGV, observable on every PR targeting9.1(including unrelated ones like9.1-release-version) and on essentially every CI job that runs the sentinel suite.The crash
Worked with Kiro CLI!