Fix module loading failure for redis module by roshkhatri · Pull Request #3374 · valkey-io/valkey

roshkhatri · 2026-03-18T02:33:37Z

Fixes #547

Fix module loading failure ([CRASH] If you have version.script file, make sure update Redis* to Valkey* #547) where modules using a GNU ld version.script that explicitly export RedisModule_OnLoad failed to load because the header remapped that symbol to ValkeyModule_OnLoad.
Remove the #define RedisModule_OnLoad ValkeyModule_OnLoad macro from src/redismodule.h and add a comment explaining why the RedisModule_OnLoad entry-point must remain unmapped, while keeping server compatibility since valkey-server probes both ValkeyModule_OnLoad and RedisModule_OnLoad.
Test by loading this minimal module hellovalkey.zip and the steps mentioned in the issue [CRASH] If you have version.script file, make sure update Redis* to Valkey* #547

codecov · 2026-03-18T05:30:49Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.92%. Comparing base (948aaf8) to head (32c85a1).

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #3374      +/-   ##
============================================
+ Coverage     76.68%   76.92%   +0.24%     
============================================
  Files           162      162              
  Lines         80729    80729              
============================================
+ Hits          61903    62098     +195     
+ Misses        18826    18631     -195

see 23 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

hpatro · 2026-03-18T07:22:49Z

@roshkhatri How did you test it ?

roshkhatri · 2026-03-18T16:24:53Z

I tested this by loading a the minimal module that is mentioned on the issue #547

sarthakaggarwal97

Looks good!

hpatro · 2026-03-18T18:19:32Z

@roshkhatri Shall we add it as a test then ?

I believe this would be useful for folks to load a Redis compatible module to Valkey if other API(s) are available.

Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

) Signed-off-by: Harkrishn Patro <bunty.hari@gmail.com> Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

Probably added by mistake during some merge of valkey-io#1566 Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

Nikhil-Manglore

LGTM

PingXie

Lgtm. One nit on the makefile rule.

PingXie · 2026-05-18T02:09:44Z

 32bit:
 	$(MAKE) CFLAGS="-m32" LDFLAGS="-m32"

 %.xo: %.c ../../src/valkeymodule.h


We should add a rule to have this new test module file depend on redismodule.h

@PingXie

hellovalkey is the only test module that includes redismodule.h. The generic %.xo pattern rule only tracks valkeymodule.h, so changes to redismodule.h would not trigger a rebuild of hellovalkey.xo, defeating the purpose of this regression test for valkey-io#547. Addresses review comment from @PingXie. Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

coderabbitai · 2026-06-04T20:47:24Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 47860cea-4ab3-43c8-a245-5dce4bd6466e

📥 Commits

Reviewing files that changed from the base of the PR and between 337003a and 32c85a1.

📒 Files selected for processing (1)

tests/modules/Makefile

💤 Files with no reviewable changes (1)

tests/modules/Makefile

📝 Walkthrough

Walkthrough

Removes the RedisModule_OnLoad macro alias from the public header and documents that the server supports both ValkeyModule_OnLoad and RedisModule_OnLoad. Adds a test module (hellovalkey) with a version script and Makefile rules, and a unit test that loads and unloads the module verifying the RedisModule_OnLoad path.

Changes

Legacy module symbol support

Layer / File(s)	Summary
API contract: remove macro alias `src/redismodule.h`	Removes the macro mapping `RedisModule_OnLoad` to `ValkeyModule_OnLoad`, replacing it with a comment block explaining the server loads both symbol names natively.
Test module implementation with build rules `tests/modules/Makefile`, `tests/modules/hellovalkey.c`, `tests/modules/hellovalkey.version`	Adds a minimal test module `hellovalkey.c` implementing `RedisModule_OnLoad`, a version script exporting only that symbol, and Makefile rules that depend on `redismodule.h` and link with a Linux-only `--version-script` when applicable.
Module load/unload test `tests/unit/moduleapi/redismodule_onload.tcl`	Tcl test normalizes the module path, conditionally checks the shared object exports `RedisModule_OnLoad` on Linux, asserts `module load` succeeds and emits the legacy init log, then asserts `module unload` succeeds.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the primary fix: resolving module loading failure when using the redis module compatibility API.
Description check	✅ Passed	The description is well-related to the changeset, referencing the linked issue and explaining the core fix of removing the macro alias.
Linked Issues check	✅ Passed	The PR successfully addresses all coding requirements from issue `#547`: removes the macro alias, maintains server compatibility, adds the hellovalkey module, and establishes build dependencies for test modules.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the module loading issue: header changes, build rule additions, test module implementation, and test coverage are all aligned with the stated objectives.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

roshkhatri · 2026-06-09T21:06:59Z

@PingXie can you please merge this?

PingXie · 2026-06-10T03:51:03Z

@roshkhatri there is a test failure?

roshkhatri · 2026-06-10T03:53:20Z

Its not related to the code:

Run mkdir -p tests/tmp
--2026-06-04 20:49:31--  https://download.valkey.io/releases/valkey-7.2.11-noble-x86_64.tar.gz
Resolving download.valkey.io (download.valkey.io)... 13.249.74.8, 13.249.74.7, 13.249.74.55, ...
Connecting to download.valkey.io (download.valkey.io)|13.249.74.8|:443... connected.
HTTP request sent, awaiting response... 403 Forbidden
2026-06-04 20:49:31 ERROR 403: Forbidden.

connection issue with GitHub and the artifacts, you can rerun this test o see if it passes

roshkhatri · 2026-06-10T06:04:37Z

@PingXie all tests passed

github-actions Bot assigned roshkhatri Mar 18, 2026

hpatro requested a review from PingXie March 18, 2026 07:18

sarthakaggarwal97 approved these changes Mar 18, 2026

View reviewed changes

roshkhatri and others added 4 commits March 20, 2026 01:10

moduleapi: keep RedisModule_OnLoad symbol unmapped

29c1ff3

Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

Add AGENTS.md file for agentic coding assistant steering (valkey-io#3371

e74eb8e

) Signed-off-by: Harkrishn Patro <bunty.hari@gmail.com> Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

remove duplicated lline (valkey-io#3379)

f24a9e6

Probably added by mistake during some merge of valkey-io#1566 Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

add test module that exports it via version script

1ab5be0

Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

roshkhatri force-pushed the fix-crash-#547 branch from 09c8807 to 1ab5be0 Compare March 20, 2026 01:11

roshkhatri and others added 2 commits March 20, 2026 01:12

Merge branch 'unstable' into fix-crash-valkey-io#547

daff79f

Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

Merge branch 'unstable' into fix-crash-valkey-io#547

fa6f5f2

Nikhil-Manglore approved these changes Mar 30, 2026

View reviewed changes

PingXie approved these changes May 18, 2026

View reviewed changes

roshkhatri requested a review from PingXie June 5, 2026 20:59

PingXie approved these changes Jun 7, 2026

View reviewed changes

Merge branch 'unstable' into fix-crash-valkey-io#547

32c85a1

PingXie merged commit 719268e into valkey-io:unstable Jun 10, 2026
63 checks passed

Uh oh!

Conversation

roshkhatri commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hpatro commented Mar 18, 2026

Uh oh!

roshkhatri commented Mar 18, 2026

Uh oh!

sarthakaggarwal97 left a comment

Choose a reason for hiding this comment

Uh oh!

hpatro commented Mar 18, 2026

Uh oh!

Nikhil-Manglore left a comment

Choose a reason for hiding this comment

Uh oh!

PingXie left a comment

Choose a reason for hiding this comment

Uh oh!

PingXie May 18, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

roshkhatri commented Jun 9, 2026

Uh oh!

PingXie commented Jun 10, 2026

Uh oh!

roshkhatri commented Jun 10, 2026

Uh oh!

roshkhatri commented Jun 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

roshkhatri commented Mar 18, 2026 •

edited

Loading

codecov Bot commented Mar 18, 2026 •

edited

Loading

coderabbitai Bot commented Jun 4, 2026 •

edited

Loading