Move Lua scripting engine into a Valkey module #2858

rjd15372 · 2025-11-20T10:06:18Z

This PR restructures the Lua scripting functionality by extracting
it from the core Valkey server into a separate Valkey module. This change
enables the possibility of a backwards compatible Lua engine upgrade, as well
as, the flexibility in building Valkey without the Lua engine.

Important: from a user's point of view, there's no difference in using
the EVAL of FUNCTION/FCALL scripts. This PR is fully backward compatible
with respect to the public API.

The main code change is the move and adaptation of the Lua engine source
files from src/lua to src/modules/lua. The original Lua engine code is
adapted to use the module API to compile and execute scripts.

The main difference between the original code and the new, is the
serialization and deserialization of Valkey RESP values into, and from,
Lua values. While in the original implementation the parsing of RESP values
was done directly from the client buffer, in the new implementation the
parsing is done from the ValkeyModuleCallReply object and respective API.

The Makefile and CMake build systems were also updated to build and
integrate the new Lua engine module, within the Valkey server build
workflow.
When the Valkey server is built, the Lua engine module is also built,
and, the Lua module is loaded automatically by the server upon startup.
When running make install the Lua engine module is installed in the
default system library directory.
There's a new build option, called BUILD_LUA, that if set to no allows to
build Valkey server without building the Lua engine.

This modular architecture enables future development of additional Lua
engine modules with newer Lua versions that can be loaded alongside the
current engine, facilitating gradual migration paths for users.

Fixes: #1627

rjd15372 · 2025-11-20T10:07:03Z

This PR is rebased on top of #2836 . When #2836 is merged, I'll update this PR.

This commit restructures the Lua scripting functionality by extracting it from the core Valkey server into a separate Valkey module. This change enables the possibility of a backwards compatible Lua engine upgrade, as well as, the flexibility in building Valkey without the Lua engine. **Important**: from a user's point of view, there's no difference in using the `EVAL` of `FUNCTION/FCALL` scripts. This PR is fully backward compatible with respect to the public API. The main code change is the move and adaptation of the Lua engine source files from `src/lua` to `src/modules/lua`. The original Lua engine code is adapted to use the module API to compile and execute scripts. The main difference between the original code and the new, is the serialization and deserialization of Valkey RESP values into, and from, Lua values. While in the original implementation the parsing of RESP values was done directly from the client buffer, in the new implementation the parsing is done from the `ValkeyModuleCallReply` object and respective API. The Makefile and CMake build systems were also updated to build and integrate the new Lua engine module, within the Valkey server build workflow. When the Valkey server is built, the Lua engine module is also built, and, the Lua module is loaded automatically by the server upon startup. When running `make install` the Lua engine module is installed in the default system library directory. There's a new build option, called `WITHOUT_LUA`, that if set allows to build Valkey server without building the Lua engine. This modular architecture enables future development of additional Lua engine modules with newer Lua versions that can be loaded alongside the current engine, facilitating gradual migration paths for users. Signed-off-by: Ricardo Dias <[email protected]>

zuiderkwast · 2025-11-20T10:42:39Z

Wow!

Some initial comments:

For the build option, why not use the name BUILD_LUA (default yes), to align with the build options BUILD_TLS and BUILD_RDMA?
For backward compatibility, the lua module needs to load by default. To disable this auto-loading, we need a new config, don't we? I can't see it mentioned here.
Can the lua module be unloaded using MODULE UNLOAD?

rjd15372 · 2025-11-20T10:46:50Z

For the build option, why not use the name BUILD_LUA (default yes), to align with the build options BUILD_TLS and BUILD_RDMA?

Sure, that makes more sense. I'll update the PR.

For backward compatibility, the lua module needs to load by default. To disable this auto-loading, we need a new config, don't we? I can't see it mentioned here.

I was thinking that if the lua module is built then it would always be loaded by default. But we can add a new config to disable auto-loading even if the lua module is built.

Can the lua module be unloaded using MODULE UNLOAD?

Yes

zuiderkwast · 2025-11-20T11:29:47Z

I was thinking that if the lua module is built then it would always be loaded by default. But we can add a new config to disable auto-loading even if the lua module is built.

Yeah, we can discuss it with the core team.

I believe in many contexts, such as in pre-built containers, the module is already built. Many users don't build their own binaries.

rjd15372 · 2025-11-20T11:32:05Z

I think it makes sense to have that config option to disable auto-load.

Signed-off-by: Ricardo Dias <[email protected]>

rjd15372 · 2025-11-20T14:39:15Z

I think it makes sense to have that config option to disable auto-load.

@zuiderkwast I vote for implementing such option in a follow up PR. This PR is already huge to review.

Signed-off-by: Ricardo Dias <[email protected]>

codecov · 2025-11-21T11:58:38Z

Codecov Report

❌ Patch coverage is 81.03448% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.87%. Comparing base (48e0cbb) to head (283ac27).

Files with missing lines	Patch %	Lines
src/module.c	74.28%	9 Missing ⚠️
src/scripting_engine.c	92.30%	1 Missing ⚠️
src/server.c	87.50%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2858      +/-   ##
============================================
+ Coverage     72.47%   73.87%   +1.40%     
============================================
  Files           129      125       -4     
  Lines         70537    68775    -1762     
============================================
- Hits          51121    50810     -311     
+ Misses        19416    17965    -1451

Files with missing lines	Coverage Δ
src/config.c	`78.76% <ø> (+0.32%)`	⬆️
src/eval.c	`87.46% <100.00%> (ø)`
src/module.h	`0.00% <ø> (ø)`
src/replication.c	`86.01% <ø> (-0.05%)`	⬇️
src/script.c	`89.03% <ø> (+8.89%)`	⬆️
src/scripting_engine.c	`56.88% <92.30%> (+3.51%)`	⬆️
src/server.c	`88.69% <87.50%> (+0.29%)`	⬆️
src/module.c	`25.48% <74.28%> (+15.71%)`	⬆️

... and 22 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.

Signed-off-by: Ricardo Dias <[email protected]>

zuiderkwast

Just some initial comments. I'll do another pass later. The change is not as huge as the +/- numbers indicate. Many lines are moved to other files.

The build options should be mentioned in the README.md.

src/modules/lua/engine_lua.c

src/modules/lua/list.c

src/modules/lua/script_lua.c

Signed-off-by: Ricardo Dias <[email protected]>

src/scripting_engine.c

Signed-off-by: Ricardo Dias <[email protected]>

zuiderkwast

This looks pretty solid in general.

My main concerns are about the build-time changes. Is there a risk that it will break compilation or installation for any user?

If Valkey is build with BUILD_LUA=no, then it doesn't automatically load a lua module even if such module is installed in the system later. Maybe it's not bad, but I just want to mention it.

@valkey-io/valkey-committers Does anyone else want to take a look, especially on the compile-time changes (Makefiles etc.)?

src/Makefile

src/server.c

rjd15372 · 2025-11-25T11:48:07Z

My main concerns are about the build-time changes. Is there a risk that it will break compilation or installation for any user?

I tried my best to not break it, but a another pair of eyes is needed to improve confidence on the changes.

If Valkey is build with BUILD_LUA=no, then it doesn't automatically load a lua module even if such module is installed in the system later. Maybe it's not bad, but I just want to mention it.

Right. But if later someone installs the module separately, it can add a loadmodule line to valkey.conf to load it automatically.

Signed-off-by: Ricardo Dias <[email protected]>

src/config.c

Signed-off-by: Ricardo Dias <[email protected]>

src/module.c

src/CMakeLists.txt

Signed-off-by: Ricardo Dias <[email protected]>

src/modules/lua/engine_lua.c

zuiderkwast · 2025-12-09T11:49:26Z

Do you know why the external tests are failing? I can see this in the logs:

Notice: nested start_server statements in external server mode, test must be aware of that!

When I check tests/unit/type/stream-cgroups.tcl, I see there's a nested start_server on line 1012, but it's not new. Anyway, I don't think nested start_server works on external server tests so we should add the tag external:skip, but in a separate PR. But I wonder why it started failing now and not in unstable.

rjd15372 · 2025-12-09T11:55:46Z

Do you know why the external tests are failing? I can see this in the logs:

I'm investigating. This is not the first time that fails in the same assertion, it should be something caused by this PR.

zuiderkwast

A few random comments and questions. See below.

zuiderkwast · 2025-12-09T12:28:03Z

src/Makefile

+	LUA_MODULE_INSTALL=install-lua-module
+
+	current_dir = $(shell pwd)
+	FINAL_CFLAGS+=-DLUA_ENGINE_ENABLED -DLUA_ENGINE_LIB=libvalkeylua.so


What does the "ENGINE" part of these names add? I imagine just LUA_ENABLED and LUA_LIB are more concise and clear.

Suggested change

FINAL_CFLAGS+=-DLUA_ENGINE_ENABLED -DLUA_ENGINE_LIB=libvalkeylua.so

FINAL_CFLAGS+=-DLUA_ENABLED -DLUA_LIB=libvalkeylua.so

zuiderkwast · 2025-12-09T15:10:59Z

src/modules/lua/Makefile

+
+LIBS= $(DEPS_DIR)/lua/src/liblua.a $(DEPS_DIR)/fpconv/libfpconv.a
+SRCS= $(wildcard *.c)
+OBJS= $(SRCS:.c=.o) sha1.o rand.o


The lua module depends on sha1 and rand from Valkey and fpconv from Valkey deps? (You removed dependency to sds, adlist and stuff like ll2string though.) Is this a problem? An alternative is to provide SHA1, random and float-parsing in the module API.

Let's create a modules/lua/README.md file that explains how this module is independent (or not) to the valkey's source code and other relevant information about it. I can imagine future contributors might add dependency on valkey internals again unless there is some text describing the ideas here.

How do we handle these dependencies in an external Lua 5.4 module? Do we copy these dependencies to that module? (If the module and the core use different versions of e.g. fpconv, would it work?)

zuiderkwast · 2025-12-09T15:16:12Z

src/modules/lua/script_lua.c

+ * Based on the following article (that apparently does not provide a
+ * novel approach but only publicizes an already used technique):
+ *
+ * https://www.facebook.com/notes/facebook-engineering/three-optimization-tips-for-c/10151361643253920 */


This link doesn't seem to work anymore (or it requires login). We can skip it or use wayback machine. I found a link that works:

Suggested change

* https://www.facebook.com/notes/facebook-engineering/three-optimization-tips-for-c/10151361643253920 */

* https://web.archive.org/web/20150427221229/https://www.facebook.com/notes/facebook-engineering/three-optimization-tips-for-c/10151361643253920 */

We could also just delete the link.

zuiderkwast · 2025-12-09T15:36:27Z

src/modules/lua/debug_lua.c

+        old_s = s;
+        s = ValkeyModule_CreateStringPrintf(NULL, "%s %g", prefix, (double)lua_tonumber(lua, idx));


In the TNUMBER case, isn't there an extra space added in the format "%s %g"? Before this change, it used sdscatprintf with the format "%g" which doesn't insert a space before the number.

I see old_s is freed in the end of the function. I think the code is easier to follow if make this variable local to this case and free it locally. Same with the prefix variable, either move it to the local scope or we don't need it at all?

Suggested change

old_s = s;

s = ValkeyModule_CreateStringPrintf(NULL, "%s %g", prefix, (double)lua_tonumber(lua, idx));

ValkeyModuleString old_s = s;

s = ValkeyModule_CreateStringPrintf(NULL, "%s%g",

ValkeyModule_StringPtrLen(s, NULL),

(double)lua_tonumber(lua, idx));

ValkeyModule_FreeString(NULL, old_s);

zuiderkwast · 2025-12-09T16:13:35Z

src/modules/lua/script_lua.c


+static void _serverPanic(const char *file, int line, const char *msg, ...) {
+    fprintf(stderr, "------------------------------------------------");
+    fprintf(stderr, "!!! Software Failure.");


Suggested change

fprintf(stderr, "!!! Software Failure.");

fprintf(stderr, "!!! Software Failure. Press left mouse button to continue.");

It's from Amiga. Left button doesn't make sense anymore but it's part of the original message. Do you think it's silly to keep it?

https://en.wikipedia.org/wiki/Guru_Meditation

https://www.generationamiga.com/2023/01/03/one-of-the-weirdest-names-for-an-error-message-but-what-is-a-guru-meditation/

Alternative:

Should we add a ValkeyModule_Panic wrapper, similar to ValkeyModule_Assert?

rjd15372 requested review from madolson and zuiderkwast November 20, 2025 10:06

rjd15372 self-assigned this Nov 20, 2025

rjd15372 added the enhancement New feature or request label Nov 20, 2025

rjd15372 added this to Valkey 9.1 Nov 20, 2025

rjd15372 force-pushed the lua-module branch from 58f217a to 8f0ba6d Compare November 20, 2025 10:28

rjd15372 added 2 commits November 20, 2025 14:28

Fix test client flags

884fa12

Signed-off-by: Ricardo Dias <[email protected]>

fix build issues and use BUILD_LUA option

18b8500

Signed-off-by: Ricardo Dias <[email protected]>

rjd15372 force-pushed the lua-module branch from f40d718 to 948a554 Compare November 21, 2025 11:12

fix macos build

afbdbf5

Signed-off-by: Ricardo Dias <[email protected]>

rjd15372 force-pushed the lua-module branch from 948a554 to afbdbf5 Compare November 21, 2025 11:39

fix 32bit build

f36121f

Signed-off-by: Ricardo Dias <[email protected]>

zuiderkwast reviewed Nov 21, 2025

View reviewed changes

fix failing tests

11611c8

Signed-off-by: Ricardo Dias <[email protected]>

zuiderkwast reviewed Nov 24, 2025

View reviewed changes

src/scripting_engine.c Outdated Show resolved Hide resolved

Addressing Viktor's comments

7a83347

Signed-off-by: Ricardo Dias <[email protected]>

zuiderkwast reviewed Nov 25, 2025

View reviewed changes

src/Makefile Outdated Show resolved Hide resolved

src/server.c Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

simplify makefile logic

c491772

Signed-off-by: Ricardo Dias <[email protected]>

zuiderkwast reviewed Nov 25, 2025

View reviewed changes

src/config.c Outdated Show resolved Hide resolved

zuiderkwast reviewed Nov 25, 2025

View reviewed changes

src/config.c Outdated Show resolved Hide resolved

Renamed module shared library to libvalkeylua.so and fix cmake build

8c475a1

Signed-off-by: Ricardo Dias <[email protected]>

rjd15372 added 2 commits November 26, 2025 10:01

Updated shutdown code to unload all modules

1493e33

Signed-off-by: Ricardo Dias <[email protected]>

Fix failed tests due to module unload on shutdown

501ded5

Signed-off-by: Ricardo Dias <[email protected]>

zuiderkwast reviewed Nov 26, 2025

View reviewed changes

src/module.c Show resolved Hide resolved

src/CMakeLists.txt Outdated Show resolved Hide resolved

rjd15372 added 6 commits November 26, 2025 16:24

fix cmake

fa2b09d

Signed-off-by: Ricardo Dias <[email protected]>

moduleUnloadAllModules comment

2fd1440

Signed-off-by: Ricardo Dias <[email protected]>

remove lua-enable-insecure-api from lua module and use CONFIG GET

b551984

Signed-off-by: Ricardo Dias <[email protected]>

fix module context client for scripting engine callbacks

ea6f2a8

Signed-off-by: Ricardo Dias <[email protected]>

handle the case where CONFIG GET returns an error

303aa09

Signed-off-by: Ricardo Dias <[email protected]>

fix clang format

02586d0

Signed-off-by: Ricardo Dias <[email protected]>

zuiderkwast reviewed Nov 27, 2025

View reviewed changes

src/modules/lua/engine_lua.c Show resolved Hide resolved

zuiderkwast moved this to In Progress in Valkey 9.1 Nov 28, 2025

Merge remote-tracking branch 'upstream/unstable' into lua-module

283ac27

zuiderkwast added run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) release-notes This issue should get a line item in the release notes labels Dec 9, 2025

zuiderkwast reviewed Dec 9, 2025

View reviewed changes

	FINAL_CFLAGS+=-DLUA_ENGINE_ENABLED -DLUA_ENGINE_LIB=libvalkeylua.so
	FINAL_CFLAGS+=-DLUA_ENABLED -DLUA_LIB=libvalkeylua.so

	* https://www.facebook.com/notes/facebook-engineering/three-optimization-tips-for-c/10151361643253920 */
	* https://web.archive.org/web/20150427221229/https://www.facebook.com/notes/facebook-engineering/three-optimization-tips-for-c/10151361643253920 */

		old_s = s;
		s = ValkeyModule_CreateStringPrintf(NULL, "%s %g", prefix, (double)lua_tonumber(lua, idx));

-        old_s = s;
-        s = ValkeyModule_CreateStringPrintf(NULL, "%s %g", prefix, (double)lua_tonumber(lua, idx));
+        ValkeyModuleString old_s = s;
+        s = ValkeyModule_CreateStringPrintf(NULL, "%s%g",
+                                            ValkeyModule_StringPtrLen(s, NULL),
+                                            (double)lua_tonumber(lua, idx));
+        ValkeyModule_FreeString(NULL, old_s);

	fprintf(stderr, "!!! Software Failure.");
	fprintf(stderr, "!!! Software Failure. Press left mouse button to continue.");

Move Lua scripting engine into a Valkey module #2858

Are you sure you want to change the base?

Move Lua scripting engine into a Valkey module #2858

Uh oh!

Conversation

rjd15372 commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjd15372 commented Nov 20, 2025

Uh oh!

zuiderkwast commented Nov 20, 2025

Uh oh!

rjd15372 commented Nov 20, 2025

Uh oh!

zuiderkwast commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjd15372 commented Nov 20, 2025

Uh oh!

rjd15372 commented Nov 20, 2025

Uh oh!

codecov bot commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rjd15372 commented Nov 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zuiderkwast commented Dec 9, 2025

Uh oh!

rjd15372 commented Dec 9, 2025

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

zuiderkwast Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

zuiderkwast Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

zuiderkwast Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

zuiderkwast Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

zuiderkwast Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rjd15372 commented Nov 20, 2025 •

edited

Loading

zuiderkwast commented Nov 20, 2025 •

edited

Loading

codecov bot commented Nov 21, 2025 •

edited

Loading