Module command result callback addition by martinrvisser · Pull Request #2936 · valkey-io/valkey

martinrvisser · 2025-12-15T12:44:42Z

Add Command Result Event Notifications for Modules

Summary

Adds new server events ValkeyModuleEvent_CommandResultSuccess and ValkeyModuleEvent_CommandResultFailure for that can notify subscribed modules after command execution. This enables modules to implement audit logging, error monitoring, performance tracking, and observability without modifying core server code.
Adds new server event ValkeyModuleEvent_CommandResultACLDenied for commands rejected by ACL. Together with PR New module API event for tracking authentication attempts #2237 this covers auditing of authentication and authorisation.

Motivation

There is currently no module API to observe command outcomes after execution or to capture ACL denied commands. Modules that need audit logging or error monitoring have no mechanism to be notified when commands succeed or fail, what arguments were used, how long they took, or how many keys were modified. This feature fills that gap using the existing ValkeyModule_SubscribeToServerEvent() infrastructure.

API

Events

Event	Description
`ValkeyModuleEvent_CommandResultSuccess`	Fired after a command completes successfully
`ValkeyModuleEvent_CommandResultFailure`	Fired after a command returns an error
`ValkeyModuleEvent_CommandACLDenied`	Fired after a command is rejected by ACL

These are separate events (not sub-events), so modules can for example only subscribe to failures without incurring any callback overhead for successful commands.

Event Data: `ValkeyModuleCommandResultInfo`

The data pointer passed to the callback can be cast to ValkeyModuleCommandResultInfo:

typedef struct ValkeyModuleCommandResultInfo {
    uint64_t version;           /* Version of this structure for ABI compat. */
    const char *command_name;   /* Full command name (e.g., "SET", "CLIENT|LIST"). */
    long long duration_us;      /* Execution duration in microseconds. */
    long long dirty;            /* Number of keys modified. */
    uint64_t client_id;         /* Client ID that executed the command. */
    int is_module_client;       /* 1 if command was from RM_Call, 0 otherwise. */
    int argc;                   /* Number of command arguments. */
    ValkeyModuleString **argv;  /* Command arguments array (zero-copy, read-only). */
    int acl_deny_reason;        /* ACL_DENIED_CMD/KEY/CHANNEL/AUTH; 0 for non-ACL events */
    const char *acl_object;     /* Denied resource name (key/channel); NULL for CMD/AUTH */
} ValkeyModuleCommandResultInfoV1;

The struct is versioned (VALKEYMODULE_COMMANDRESULTINFO_VERSION) for forward-compatible API evolution.

Usage Example

/* Callback receives events for whichever event(s) you subscribed to */
void OnCommandResult(ValkeyModuleCtx *ctx, ValkeyModuleEvent eid,
                     uint64_t subevent, void *data) {
    VALKEYMODULE_NOT_USED(ctx);
    VALKEYMODULE_NOT_USED(subevent);

    ValkeyModuleCommandResultInfo *info = (ValkeyModuleCommandResultInfo *)data;
    if (info->version != VALKEYMODULE_COMMANDRESULTINFO_VERSION) return;

    int failed = (eid.id == VALKEYMODULE_EVENT_COMMAND_RESULT_FAILURE);

    /* Access fields directly */
    printf("command=%s status=%s duration=%lldus dirty=%lld client=%llu\n",
           info->command_name,
           failed ? "FAIL" : "OK",
           info->duration_us,
           info->dirty,
           info->client_id);

    /* Access argv (read-only, zero-copy) */
    for (int i = 0; i < info->argc; i++) {
        size_t len;
        const char *arg = ValkeyModule_StringPtrLen(info->argv[i], &len);
        printf("  argv[%d] = %.*s\n", i, (int)len, arg);
    }
}

/* Subscribe in ValkeyModule_OnLoad or at runtime */

/* Option A: command failures only (recommended for audit logging) */
ValkeyModule_SubscribeToServerEvent(ctx,
    ValkeyModuleEvent_CommandResultFailure, OnCommandResult);

/* Option B: command successes only */
ValkeyModule_SubscribeToServerEvent(ctx,
    ValkeyModuleEvent_CommandResultSuccess, OnCommandResult);

/* Option C: both command outcomes*/
ValkeyModule_SubscribeToServerEvent(ctx,
    ValkeyModuleEvent_CommandResultSuccess, OnCommandResult);
ValkeyModule_SubscribeToServerEvent(ctx,
    ValkeyModuleEvent_CommandResultFailure, OnCommandResult);

/* Subscribe to ACL Denied */
ValkeyModule_SubscribeToServerEvent(ctx,
        ValkeyModuleEvent_CommandResultACLDenied, onCommandResult);

/* Unsubscribe pass NULL callback */
ValkeyModule_SubscribeToServerEvent(ctx,
    ValkeyModuleEvent_CommandResultFailure, NULL);

Design Decisions

Separate events instead of sub-events: Modules subscribing only to failures have zero overhead for successful commands (~2ns listener-list check vs ~30ns callback invocation per command). This is critical since success events fire on the hot path of every command.
Stack-allocated info struct: The ValkeyModuleCommandResultInfoV1 is built on the stack ΓÇö no heap allocation per event.
Zero-copy argv: Arguments are passed directly from the client's argv array. Any integer-encoded arguments (from tryObjectEncoding() during command execution) are decoded to string-encoded objects before being passed to the callback, ensuring compatibility with ValkeyModule_StringPtrLen().
Early exit: If no modules are subscribed to any server events, the event firing function returns immediately before building the info struct.
Uses existing server event infrastructure: Follows the ValkeyModule_SubscribeToServerEvent() pattern used by all other server events, rather than introducing a new callback mechanism.

Files Changed

File	Change
`src/valkeymodule.h`	Event IDs, event constants, `ValkeyModuleCommandResultInfoV1` struct
`src/module.c`	`moduleFireCommandResultEvent()`, event documentation, event version entries
`src/module.h`	Function declaration
`src/server.c`	Call `moduleFireCommandResultEvent()` from `call()` after command execution
`src/server.c`	Call to `moduleFireCommandACLDeniedEvent` in `processCommand` after ACL rejection
`tests/modules/commandresult.c`	Test module exercising the full API
`tests/unit/moduleapi/commandresult.tcl`	Integration tests

rjd15372 · 2025-12-15T13:03:46Z

@martinrvisser you are adding benchmark code in the form of a TCL test, why did you follow this approach versus adding the code to valkey-benchmark ?

codecov · 2025-12-15T13:18:16Z

Codecov Report

❌ Patch coverage is 42.24138% with 67 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.63%. Comparing base (a34f125) to head (2d50f02).
⚠️ Report is 31 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/module.c	11.42%	62 Missing ⚠️
src/server.c	89.13%	5 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2936      +/-   ##
============================================
+ Coverage     76.41%   76.63%   +0.21%     
============================================
  Files           159      159              
  Lines         79815    80019     +204     
============================================
+ Hits          60990    61321     +331     
+ Misses        18825    18698     -127

Files with missing lines	Coverage Δ
src/module.h	`0.00% <ø> (ø)`
src/server.h	`100.00% <ø> (ø)`
src/server.c	`89.60% <89.13%> (+<0.01%)`	⬆️
src/module.c	`25.35% <11.42%> (-0.54%)`	⬇️

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

martinrvisser · 2025-12-15T14:48:40Z

@martinrvisser you are adding benchmark code in the form of a TCL test, why did you follow this approach versus adding the code to valkey-benchmark ?

valkey-benchmark is aimed at benchmarking Valkey commands where as this is specific to module implementations. There are no other module API tests in valkey-benchmark?

rjd15372 · 2025-12-16T10:14:56Z

valkey-benchmark is aimed at benchmarking Valkey commands where as this is specific to module implementations. There are no other module API tests in valkey-benchmark?

Although valkey-benchmark does not have any module specific tests, I still feel that it's the right place to put any kind of benchmark. We need to make a few changes though because we will have to build the module along with valkey-benchmark.

Since moving benchmark code to valkey-benchmark requires a significant amount of work, and is orthogonal to the main focus of this PR, which is the proposal of a new module API, I propose to remove the benchmarking code from this PR, and just keep unit tests that test the new module API. And then open a new PR with just the benchmarking part.

martinrvisser · 2026-01-08T15:28:32Z

valkey-benchmark is aimed at benchmarking Valkey commands where as this is specific to module implementations. There are no other module API tests in valkey-benchmark?

Although valkey-benchmark does not have any module specific tests, I still feel that it's the right place to put any kind of benchmark. We need to make a few changes though because we will have to build the module along with valkey-benchmark.

Since moving benchmark code to valkey-benchmark requires a significant amount of work, and is orthogonal to the main focus of this PR, which is the proposal of a new module API, I propose to remove the benchmarking code from this PR, and just keep unit tests that test the new module API. And then open a new PR with just the benchmarking part.

@rjd15372 Removed the benchmark code

rjd15372

Besides my inline comments, I also have a high level question. Should we add the restriction that a module can only register a single command result callback?

I'm not seeing a use case where a module needs to register more than one callback. And if that is the case, then we don't need to store the callbacks on a list, and we can store the module's callback in the module structure.

martinrvisser · 2026-01-14T14:27:51Z

Besides my inline comments, I also have a high level question. Should we add the restriction that a module can only register a single command result callback?

I'm not seeing a use case where a module needs to register more than one callback. And if that is the case, then we don't need to store the callbacks on a list, and we can store the module's callback in the module structure.

@rjd15372 Agreed, no immediate use case so changed this to single callback.

rjd15372 · 2026-01-15T15:42:55Z

@valkey-io/core-team can you look at this proposal?

rjd15372 · 2026-01-15T15:44:08Z

@martinrvisser please fix the check failures

dmitrypol · 2026-01-26T16:14:40Z

very interesting idea @martinrvisser . Could you please add an example in src/modules/hello* files?

Copilot

Pull request overview

This PR introduces a new Module API for command result callbacks, enabling modules to register callbacks that fire after command execution completes. This allows modules to monitor command success/failure, track execution metrics, capture command arguments and replies for audit logging, and implement observability features without requiring expensive pre-execution filters.

Changes:

Adds 11 new Module API functions for registering callbacks and accessing command execution results (status, duration, dirty keys, arguments, replies)
Implements performance optimizations via FAILURES_ONLY flag (callbacks only on errors) and NOSELF flag (skip callbacks during RM_Call)
Fixes existing bug in VM_StringPtrLen where integer-encoded strings would crash, now converts to thread-local buffer
Includes comprehensive test suite with 30+ test cases covering all API features and edge cases

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/module.c	Core implementation of command result callback API, registration/unregistration, callback invocation logic, accessor functions for status/duration/dirty/argv/reply, and fix for VM_StringPtrLen integer encoding crash
src/module.h	Added result_callback field to ValkeyModule structure and moduleCallCommandResultCallbacks function declaration
src/valkeymodule.h	Public API declarations for 11 new functions, flag definitions (FAILURES_ONLY, NOSELF, SUCCESS, FAILURE), and new type definitions (ValkeyModuleCommandResultCtx, ValkeyModuleCommandResult)
src/server.c	Integration point in call() function to invoke callbacks after command execution with timing and dirty key metrics
tests/modules/commandresult.c	Full-featured test module implementing callback registration, logging, and testing all API accessor functions
tests/unit/moduleapi/commandresult.tcl	Comprehensive integration tests covering 30+ scenarios including flags, edge cases, argv/reply capture, and unload behavior
tests/modules/Makefile	Added commandresult.so to build targets
tests/modules/CMakeLists.txt	Added commandresult module to CMake build configuration

Copilot · 2026-01-26T16:17:21Z

+ValkeyModuleCommandResult *VM_RegisterCommandResult(ValkeyModuleCtx *ctx,
+                                                    ValkeyModuleCommandResultFunc callback,
+                                                    int flags) {
+    /* Only allow one callback per module */
+    if (ctx->module->result_callback != NULL) {
+        return NULL;  /* Already registered */
+    }
+
+    /* Validate flags - only accept known flag bits */
+    int valid_flags = VALKEYMODULE_CMDRESULT_FAILURES_ONLY | 
+                      VALKEYMODULE_CMDRESULT_NOSELF;
+    if (flags & ~valid_flags) {
+        return NULL;  /* Invalid flags */
+    }
+
+    ValkeyModuleCommandResult *result = zmalloc(sizeof(*result));
+    result->module = ctx->module;
+    result->callback = callback;
+    result->flags = flags;
+    /* Record the command count at registration time to avoid firing callback
+     * for the registration command itself */
+    result->registered_at_cmd_count = server.stat_numcommands;
+
+    ctx->module->result_callback = result;
+    return result;
+}


This PR introduces a new Module API for command result callbacks, which represents a significant addition to the module system that affects core command execution paths and creates a new public API surface. According to Valkey project guidelines, this appears to require TSC (Technical Steering Committee) review as it falls under "Technical Major Decisions":

New data structures or APIs

New user-visible fields requiring long-term maintenance

Changes that affect runtime behavior on the critical path

Please confirm that TSC consensus has been reached for this feature. If not already discussed, please reference or create an Issue for TSC review and link it to this PR.

cc @core-team

Copilot · 2026-01-26T16:17:21Z

+VALKEYMODULE_API ValkeyModuleCommandResult *(*ValkeyModule_RegisterCommandResult)(ValkeyModuleCtx *ctx,
+                                                                                  ValkeyModuleCommandResultFunc cb,
+                                                                                  int flags) VALKEYMODULE_ATTR;
+VALKEYMODULE_API int (*ValkeyModule_UnregisterCommandResult)(ValkeyModuleCtx *ctx,
+                                                             ValkeyModuleCommandResult *result) VALKEYMODULE_ATTR;
+VALKEYMODULE_API int (*ValkeyModule_CommandResultGetStatus)(ValkeyModuleCommandResultCtx *rctx) VALKEYMODULE_ATTR;
+VALKEYMODULE_API const char *(*ValkeyModule_CommandResultGetCommandName)(ValkeyModuleCommandResultCtx *rctx)
+    VALKEYMODULE_ATTR;
+VALKEYMODULE_API long long (*ValkeyModule_CommandResultGetDuration)(ValkeyModuleCommandResultCtx *rctx) VALKEYMODULE_ATTR;
+VALKEYMODULE_API long long (*ValkeyModule_CommandResultGetDirty)(ValkeyModuleCommandResultCtx *rctx) VALKEYMODULE_ATTR;
+VALKEYMODULE_API unsigned long long (*ValkeyModule_CommandResultGetClientId)(ValkeyModuleCommandResultCtx *rctx)
+    VALKEYMODULE_ATTR;
+VALKEYMODULE_API int (*ValkeyModule_CommandResultGetArgv)(ValkeyModuleCommandResultCtx *rctx,
+                                                          ValkeyModuleString ***argv,
+                                                          int *argc) VALKEYMODULE_ATTR;
+VALKEYMODULE_API size_t (*ValkeyModule_CommandResultGetReplySize)(ValkeyModuleCommandResultCtx *rctx) VALKEYMODULE_ATTR;
+VALKEYMODULE_API const char *(*ValkeyModule_CommandResultGetReplyProto)(ValkeyModuleCommandResultCtx *rctx,
+                                                                        size_t *len) VALKEYMODULE_ATTR;
+VALKEYMODULE_API ValkeyModuleCallReply *(*ValkeyModule_CommandResultCreateReply)(ValkeyModuleCommandResultCtx *rctx)
+    VALKEYMODULE_ATTR;


This PR introduces new user-facing Module APIs (ValkeyModule_RegisterCommandResult, ValkeyModule_CommandResultGet*, etc.) that change module capabilities. These changes should be documented in the valkey-doc repository at https://github.com/valkey-io/valkey-doc.

Please ensure documentation is added or create an issue in valkey-doc to track this work. The documentation should explain:

The new command result callback API

Usage examples for common scenarios (error monitoring, audit logging, performance tracking)

Performance considerations and best practices (e.g., using FAILURES_ONLY flag)

API limitations and caveats

Copilot · 2026-01-26T16:17:21Z

+
+    listIter li;
+    listNode *ln;
+    listRewind(c->reply, &li);
+    while ((ln = listNext(&li)) != NULL) {
+        clientReplyBlock *block = listNodeValue(ln);
+        proto = sdscatlen(proto, block->buf, block->used);


Memory leak potential if sdscatlen fails during iteration. If any sdscatlen call fails and returns NULL, the function should free the proto sds string before returning NULL. Currently, if sdscatlen fails, proto would be leaked.

Consider adding error checking after sdscatlen and cleaning up proto on failure.

Suggested change

listIter li;

listNode *ln;

listRewind(c->reply, &li);

while ((ln = listNext(&li)) != NULL) {

clientReplyBlock *block = listNodeValue(ln);

proto = sdscatlen(proto, block->buf, block->used);

if (proto == NULL) {

return NULL;

}

listIter li;

listNode *ln;

listRewind(c->reply, &li);

while ((ln = listNext(&li)) != NULL) {

clientReplyBlock *block = listNodeValue(ln);

sds new_proto = sdscatlen(proto, block->buf, block->used);

if (new_proto == NULL) {

sdsfree(proto);

return NULL;

}

proto = new_proto;

murphyjacob4 · 2026-01-26T16:20:06Z

+ * Returns a ValkeyModuleCommandResult pointer that can be used with
+ * ValkeyModule_UnregisterCommandResult().
+ */
+ValkeyModuleCommandResult *VM_RegisterCommandResult(ValkeyModuleCtx *ctx,


Any reason not to use VM_SubscribeToServerEvent? This could just be a new event type, right?

@murphyjacob4 SubscribeToServerEvent was not used because :

server events are designed to be used for infrequent system events rather than commands. For this reason CommandFilter isn't part of ServerEvent either.

for the audit requirement, the FAILURES_ONLY flag is crucial. Currently ServerEvents don't support flags.

server events go through each listener for each command and callback wheras with commandresult we skip early depending on the flags

serverEvents callbacks receive a ValkeyModuleEvent + void *data so lacks typed accessors like GetArgv

server events are designed to be used for infrequent system events rather than commands. For this reason CommandFilter isn't part of ServerEvent either.

Yeah I think CommandFilter does a lot more than just notify. It allows modules to rewrite the args too. So not exactly a great parallel. Regarding the design for infrequent system events - this is true that although you can subscribe to broad categories, more refined subscriptions aren't possible. The existing server events are a trickle and command callbacks will be a firehose

for the audit requirement, the FAILURES_ONLY flag is crucial. Currently ServerEvents don't support flags.

That makes sense - you need the option to select just a subset of commands.

server events go through each listener for each command and callback wheras with commandresult we skip early depending on the flags

Yeah I guess this is a limitation of the current server events API.

serverEvents callbacks receive a ValkeyModuleEvent + void *data so lacks typed accessors like GetArgv

Although it is void* you cast it to a struct that is defined in valkeymodule.h and the struct is versioned so that we don't need to introduce new APIs for each new field.

SubscribeToServerEvent was not used

So it sounds like the primary reason is:

Concerns around efficiency

Ability to scope notifications down (failures only, or all)

I guess 2 could be modeled as two different subscriptions (one for failure, one for non-failure).

I am mostly hesitant to add a bunch of dedicated APIs since it feels like we should only really maintain one "server callback" API system. I'm not convinced we necessarily need a dedicated one for commands. If the existing "server callback" API needs work (e.g. to support notification masks), I would be more inclined to add a new way to register callbacks with such masks rather than adding a new API for each notification type.

I think if we had a high-performance server callback API with support for sub-event masks it sounds like this is exactly what you want. Maybe we should just build that, so that we don't rebuild this infrastructure for the next high-volume event requirement? What do you think?

@murphyjacob4 @zuiderkwast maintaining one callback system makes sense even if currently serverEvents is mainly used for infrequent events.

So we would add VALKEYMODULE_EVENT_COMMAND_RESULT to ValkeyModule_SubscribeToServerEvent with subevents:

/* Sub-events */ #define VALKEYMODULE_SUBEVENT_COMMAND_RESULT_SUCCESS 0 #define VALKEYMODULE_SUBEVENT_COMMAND_RESULT_FAILURE 1

Since the main requirement for this PR is for auditing, I propose we minimise the API to :

VM_CommandResultGetCommandName(data) : command name

VM_CommandResultGetDuration(data) : execution duration in microseconds

VM_CommandResultGetDirty(data) : number of keys modified

VM_CommandResultGetClientId(data) : client ID

VM_CommandResultIsModuleClient(data) : if command was from RM_Call

VM_CommandResultGetArgv(data, &argv, &argc) : original command arguments (zero-copy)

Since we're following the server events approach, why don't we just expose the structure with the event fields, like we do for other events, instead of having the above accessory functions?

I also think that improving the efficiency/performance of the server events system can be done in a different PR.

madolson · 2026-01-26T16:41:32Z

    /* Update failed command calls if required. */

-    if (!incrCommandStatsOnError(real_cmd, ERROR_COMMAND_FAILED) && c->deferred_reply_errors) {
+    int command_failed = incrCommandStatsOnError(real_cmd, ERROR_COMMAND_FAILED);


There are some cases where commands finish but don't go through this call back. For example, ACL errors, Clustering errors, module command unblocks. We should decide if that should flow through.

@madolson @rjd15372 added the ACL Denied event

Other than ACL, there are quite a few scenarios, e.g. if you look at processCommand you can see all the rejected scenarios where we call reject command.

I think it is fine if not all of these are captured for now. But I guess ACL is just one class of "rejected" command. Do you think we should extend the ACL even to be a "rejected" event? I guess for now we can just explicitly document that these other rejections don't have module events yet.

@murphyjacob4 agreed there are quite a few "rejected" classes and from an audit perspective these would be of interest. Include these in this PR (since it was aimed at ACL) or a separate?

I think the question is two-fold:

Is the API we are exposing extensible enough to add the other rejected classes? E.g. instead of:

ValkeyModuleEvent_CommandResultSuccess | Fired after a command completes successfully ValkeyModuleEvent_CommandResultFailure | Fired after a command returns an error ValkeyModuleEvent_CommandACLDenied | Fired after a command is rejected by ACL

Should it be:

ValkeyModuleEvent_CommandResultSuccess | Fired after a command completes successfully ValkeyModuleEvent_CommandResultFailure | Fired after a command returns an error ValkeyModuleEvent_CommandResultRejected | Fired after a command is rejected

And we can use the subevent to indicate what type of rejected notification?

Are we structuring the code such that future module callbacks won't be missed? There are a lot of exit paths in the function for rejecting commands. AFAIU they all trigger rejectCommand. Perhaps we should make the module event firing a side effect of that call?

If we go with the suggestion in 1, I am okay with just firing a generic "rejected" event for non-ACL reject scenarios and we can later add classifications for them.

Re: 1 the second option is a logical and clean API with the subevents denoting the type

ValkeyModuleEvent_CommandResultSuccess | Fired after a command completes successfully ValkeyModuleEvent_CommandResultFailure | Fired after a command returns an error ValkeyModuleEvent_CommandResultRejected | Fired after a command is rejected

Re: 2 rejectCommand only gets the client struct and error msg string so lacks the subevent. That begs the question, should errors have a type i.e. authRequired and so on? Since that's perhaps too big a change to consider here, we can go with a wrapper function in processCommand which does the rejectCommand as well as moduleFireCommandRejectedEvent with the subevent.

For 2, it is just a suggestion, I'm also fine if we just call the function in each return path for now. Alternatively we could parameterize rejectCommand with the subevent, or use a goto or something to goto rejected in the processCommand function. I don't have strong opinions, will leave it up to you. If wrapping isn't too much work, that is also viable, although I don't want to force a large refactor if it isn't needed.

@murphyjacob4 implemented the general ValkeyModuleEvent_CommandResultRejected with subevents

I like it! We should get one more round of approval from the TSC with the new API

madolson · 2026-01-26T16:44:23Z

Core meeting. The audit logging functionality seems well defined. The other more generic functionality about parsing the outputs is not as clear, do we strictly need it? Let's update the proposal just to have what is strictly needed for auditing logging and see if we can evaluate server events instead of a new eventing system. We can keep the new eventing system if there are performance concerns with using the server event system.

martinrvisser · 2026-01-26T21:07:41Z

very interesting idea @martinrvisser . Could you please add an example in src/modules/hello* files?

@dmitrypol there is a full example in tests/modules/commandresult.c

…ULTI command - Replace "REDIRECT"/"ERR" examples with "CLUSTERDOWN"/"NOMULTI" in docblock - Update moduleFireCommandRejectedEvent comment to use "NOMULTI" example - Fix NOMULTI test to use MULTI-inside-MULTI (has CMD_NO_MULTI flag) instead of SUBSCRIBE which gets queued rather than rejected - Fix command_name -> command dict key in NOMULTI/PUBSUB/NOREPLICAS tests Signed-off-by: martinrvisser <mvisser@hotmail.com>

murphyjacob4

Thank you for your patience! This module API is one of the harder ones to design, so I have some additional points of discussion.

murphyjacob4 · 2026-04-15T15:51:53Z

        rejectCommandFormat(c, "Command '%s' not allowed inside a transaction", c->cmd->fullname);
+        moduleFireCommandRejectedEvent(c, VALKEYMODULE_SUBEVENT_COMMAND_RESULT_REJECTED_OTHER, -1, "NOMULTI");


Would it be better to just take the whole reply we are sending the user and just pass that to the module? That way we don't special codes ("NOMULTI") - the module could just parse the rejection string and categorize it.

That makes perfect sense, thanks

…error message for non-ACL Signed-off-by: martinrvisser <mvisser@hotmail.com>

Signed-off-by: martinrvisser <mvisser@hotmail.com>

murphyjacob4

I think we landed in a good place. Thanks @martinrvisser for your patience

I think core team approved it per this comment:

Discussed in the meeting to simplify and just return the error codes. If it's updated this week we will merge it in for RC2.

But will mention the team here so we are all aware @valkey-io/core-team

murphyjacob4 · 2026-04-20T18:56:26Z

+        sds pubsub_err = sdscatprintf(sdsempty(),
+                                      "Can't execute '%s': only (P|S)SUBSCRIBE / "
+                                      "(P|S)UNSUBSCRIBE / PING / QUIT / RESET are allowed in this context",
+                                      c->cmd->fullname);
+        sdsmapchars(pubsub_err, "\r\n", "  ", 2);


nit: Do you think it would be cleaner to just add the moduleFireCommandRejectedEvent to rejectCommand and rejectCommandSds (which is also used by rejectCommandFormat) - that way the call sites will remain unchanged? Also would mean we can keep the sdsmapchars inside the helper for rejectCommandFormat

So effectively make rejectCommandSds the same as what processCommandRejectSds does now

I guess AUTH uses rejectCommand as well, so maybe we just add a flag to rejectCommand for notify_modules, so the AUTH paths can specify false and do their own notification?

murphyjacob4 · 2026-04-20T18:59:29Z

 /* Registered filters */
 static list *moduleCommandFilters;

+


nit: unneeded extra newline

…and and rejectCommandSds Signed-off-by: martinrvisser <mvisser@hotmail.com>

sarthakaggarwal97 · 2026-04-21T17:19:26Z

I am seeing test failures in PR CIs post rebasing with unstable:

## Add Command Result Event Notifications for Modules ### Summary 1. Adds new server events `ValkeyModuleEvent_CommandResultSuccess` and `ValkeyModuleEvent_CommandResultFailure` for that can notify subscribed modules after command execution. This enables modules to implement audit logging, error monitoring, performance tracking, and observability without modifying core server code. 2. Adds new server event `ValkeyModuleEvent_CommandResultACLDenied` for commands rejected by ACL. Together with PR valkey-io#2237 this covers auditing of authentication and authorisation. ### Motivation There is currently no module API to observe command outcomes after execution or to capture ACL denied commands. Modules that need audit logging or error monitoring have no mechanism to be notified when commands succeed or fail, what arguments were used, how long they took, or how many keys were modified. This feature fills that gap using the existing `ValkeyModule_SubscribeToServerEvent()` infrastructure. ### API #### Events | Event | Description | |---|---| | `ValkeyModuleEvent_CommandResultSuccess` | Fired after a command completes successfully | | `ValkeyModuleEvent_CommandResultFailure` | Fired after a command returns an error | | `ValkeyModuleEvent_CommandACLDenied` | Fired after a command is rejected by ACL | These are separate events (not sub-events), so modules can for example only subscribe to failures without incurring any callback overhead for successful commands. #### Event Data: `ValkeyModuleCommandResultInfo` The `data` pointer passed to the callback can be cast to `ValkeyModuleCommandResultInfo`: ```c typedef struct ValkeyModuleCommandResultInfo { uint64_t version; /* Version of this structure for ABI compat. */ const char *command_name; /* Full command name (e.g., "SET", "CLIENT|LIST"). */ long long duration_us; /* Execution duration in microseconds. */ long long dirty; /* Number of keys modified. */ uint64_t client_id; /* Client ID that executed the command. */ int is_module_client; /* 1 if command was from RM_Call, 0 otherwise. */ int argc; /* Number of command arguments. */ ValkeyModuleString **argv; /* Command arguments array (zero-copy, read-only). */ int acl_deny_reason; /* ACL_DENIED_CMD/KEY/CHANNEL/AUTH; 0 for non-ACL events */ const char *acl_object; /* Denied resource name (key/channel); NULL for CMD/AUTH */ } ValkeyModuleCommandResultInfoV1; ``` The struct is versioned (`VALKEYMODULE_COMMANDRESULTINFO_VERSION`) for forward-compatible API evolution. ### Usage Example ```c /* Callback receives events for whichever event(s) you subscribed to */ void OnCommandResult(ValkeyModuleCtx *ctx, ValkeyModuleEvent eid, uint64_t subevent, void *data) { VALKEYMODULE_NOT_USED(ctx); VALKEYMODULE_NOT_USED(subevent); ValkeyModuleCommandResultInfo *info = (ValkeyModuleCommandResultInfo *)data; if (info->version != VALKEYMODULE_COMMANDRESULTINFO_VERSION) return; int failed = (eid.id == VALKEYMODULE_EVENT_COMMAND_RESULT_FAILURE); /* Access fields directly */ printf("command=%s status=%s duration=%lldus dirty=%lld client=%llu\n", info->command_name, failed ? "FAIL" : "OK", info->duration_us, info->dirty, info->client_id); /* Access argv (read-only, zero-copy) */ for (int i = 0; i < info->argc; i++) { size_t len; const char *arg = ValkeyModule_StringPtrLen(info->argv[i], &len); printf(" argv[%d] = %.*s\n", i, (int)len, arg); } } /* Subscribe in ValkeyModule_OnLoad or at runtime */ /* Option A: command failures only (recommended for audit logging) */ ValkeyModule_SubscribeToServerEvent(ctx, ValkeyModuleEvent_CommandResultFailure, OnCommandResult); /* Option B: command successes only */ ValkeyModule_SubscribeToServerEvent(ctx, ValkeyModuleEvent_CommandResultSuccess, OnCommandResult); /* Option C: both command outcomes*/ ValkeyModule_SubscribeToServerEvent(ctx, ValkeyModuleEvent_CommandResultSuccess, OnCommandResult); ValkeyModule_SubscribeToServerEvent(ctx, ValkeyModuleEvent_CommandResultFailure, OnCommandResult); /* Subscribe to ACL Denied */ ValkeyModule_SubscribeToServerEvent(ctx, ValkeyModuleEvent_CommandResultACLDenied, onCommandResult); /* Unsubscribe pass NULL callback */ ValkeyModule_SubscribeToServerEvent(ctx, ValkeyModuleEvent_CommandResultFailure, NULL); ``` ### Design Decisions - **Separate events instead of sub-events**: Modules subscribing only to failures have zero overhead for successful commands (~2ns listener-list check vs ~30ns callback invocation per command). This is critical since success events fire on the hot path of every command. - **Stack-allocated info struct**: The `ValkeyModuleCommandResultInfoV1` is built on the stack ΓÇö no heap allocation per event. - **Zero-copy argv**: Arguments are passed directly from the client's argv array. Any integer-encoded arguments (from `tryObjectEncoding()` during command execution) are decoded to string-encoded objects before being passed to the callback, ensuring compatibility with `ValkeyModule_StringPtrLen()`. - **Early exit**: If no modules are subscribed to any server events, the event firing function returns immediately before building the info struct. - **Uses existing server event infrastructure**: Follows the `ValkeyModule_SubscribeToServerEvent()` pattern used by all other server events, rather than introducing a new callback mechanism. ### Files Changed | File | Change | |---|---| | `src/valkeymodule.h` | Event IDs, event constants, `ValkeyModuleCommandResultInfoV1` struct | | `src/module.c` | `moduleFireCommandResultEvent()`, event documentation, event version entries | | `src/module.h` | Function declaration | | `src/server.c` | Call `moduleFireCommandResultEvent()` from `call()` after command execution | | `src/server.c` | Call to `moduleFireCommandACLDeniedEvent` in `processCommand` after ACL rejection | | `tests/modules/commandresult.c` | Test module exercising the full API | | `tests/unit/moduleapi/commandresult.tcl` | Integration tests | --------- Signed-off-by: martinrvisser <mvisser@hotmail.com> Signed-off-by: martinrvisser <martinrvisser@users.noreply.github.com> Co-authored-by: Ricardo Dias <rjd15372@gmail.com>

…unload (#3545) This follows up on the commandresult API work and fixes cleanup around unsubscribe and module unload. The main issue was that command-result event listeners could leave stale state behind. On unload, we removed the listeners themselves but didn’t fully update the fast-path listener counters. Separately, unsubscribing with a NULL callback could behave badly if the listener wasn’t present anymore. In practice, that meant later commands could still walk into command-result event handling after the module was supposed to be cleaned up. Failed in Daily as well yesterday: https://github.com/valkey-io/valkey/actions/runs/24753491944/job/72421581610#step:10:852 Related Failures: #2936 (comment) --------- Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

…unload (valkey-io#3545) This follows up on the commandresult API work and fixes cleanup around unsubscribe and module unload. The main issue was that command-result event listeners could leave stale state behind. On unload, we removed the listeners themselves but didn’t fully update the fast-path listener counters. Separately, unsubscribing with a NULL callback could behave badly if the listener wasn’t present anymore. In practice, that meant later commands could still walk into command-result event handling after the module was supposed to be cleaned up. Failed in Daily as well yesterday: https://github.com/valkey-io/valkey/actions/runs/24753491944/job/72421581610#step:10:852 Related Failures: valkey-io#2936 (comment) --------- Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

## Add Command Result Event Notifications for Modules ### Summary 1. Adds new server events `ValkeyModuleEvent_CommandResultSuccess` and `ValkeyModuleEvent_CommandResultFailure` for that can notify subscribed modules after command execution. This enables modules to implement audit logging, error monitoring, performance tracking, and observability without modifying core server code. 2. Adds new server event `ValkeyModuleEvent_CommandResultACLDenied` for commands rejected by ACL. Together with PR #2237 this covers auditing of authentication and authorisation. ### Motivation There is currently no module API to observe command outcomes after execution or to capture ACL denied commands. Modules that need audit logging or error monitoring have no mechanism to be notified when commands succeed or fail, what arguments were used, how long they took, or how many keys were modified. This feature fills that gap using the existing `ValkeyModule_SubscribeToServerEvent()` infrastructure. ### API #### Events | Event | Description | |---|---| | `ValkeyModuleEvent_CommandResultSuccess` | Fired after a command completes successfully | | `ValkeyModuleEvent_CommandResultFailure` | Fired after a command returns an error | | `ValkeyModuleEvent_CommandACLDenied` | Fired after a command is rejected by ACL | These are separate events (not sub-events), so modules can for example only subscribe to failures without incurring any callback overhead for successful commands. #### Event Data: `ValkeyModuleCommandResultInfo` The `data` pointer passed to the callback can be cast to `ValkeyModuleCommandResultInfo`: ```c typedef struct ValkeyModuleCommandResultInfo { uint64_t version; /* Version of this structure for ABI compat. */ const char *command_name; /* Full command name (e.g., "SET", "CLIENT|LIST"). */ long long duration_us; /* Execution duration in microseconds. */ long long dirty; /* Number of keys modified. */ uint64_t client_id; /* Client ID that executed the command. */ int is_module_client; /* 1 if command was from RM_Call, 0 otherwise. */ int argc; /* Number of command arguments. */ ValkeyModuleString **argv; /* Command arguments array (zero-copy, read-only). */ int acl_deny_reason; /* ACL_DENIED_CMD/KEY/CHANNEL/AUTH; 0 for non-ACL events */ const char *acl_object; /* Denied resource name (key/channel); NULL for CMD/AUTH */ } ValkeyModuleCommandResultInfoV1; ``` The struct is versioned (`VALKEYMODULE_COMMANDRESULTINFO_VERSION`) for forward-compatible API evolution. ### Usage Example ```c /* Callback receives events for whichever event(s) you subscribed to */ void OnCommandResult(ValkeyModuleCtx *ctx, ValkeyModuleEvent eid, uint64_t subevent, void *data) { VALKEYMODULE_NOT_USED(ctx); VALKEYMODULE_NOT_USED(subevent); ValkeyModuleCommandResultInfo *info = (ValkeyModuleCommandResultInfo *)data; if (info->version != VALKEYMODULE_COMMANDRESULTINFO_VERSION) return; int failed = (eid.id == VALKEYMODULE_EVENT_COMMAND_RESULT_FAILURE); /* Access fields directly */ printf("command=%s status=%s duration=%lldus dirty=%lld client=%llu\n", info->command_name, failed ? "FAIL" : "OK", info->duration_us, info->dirty, info->client_id); /* Access argv (read-only, zero-copy) */ for (int i = 0; i < info->argc; i++) { size_t len; const char *arg = ValkeyModule_StringPtrLen(info->argv[i], &len); printf(" argv[%d] = %.*s\n", i, (int)len, arg); } } /* Subscribe in ValkeyModule_OnLoad or at runtime */ /* Option A: command failures only (recommended for audit logging) */ ValkeyModule_SubscribeToServerEvent(ctx, ValkeyModuleEvent_CommandResultFailure, OnCommandResult); /* Option B: command successes only */ ValkeyModule_SubscribeToServerEvent(ctx, ValkeyModuleEvent_CommandResultSuccess, OnCommandResult); /* Option C: both command outcomes*/ ValkeyModule_SubscribeToServerEvent(ctx, ValkeyModuleEvent_CommandResultSuccess, OnCommandResult); ValkeyModule_SubscribeToServerEvent(ctx, ValkeyModuleEvent_CommandResultFailure, OnCommandResult); /* Subscribe to ACL Denied */ ValkeyModule_SubscribeToServerEvent(ctx, ValkeyModuleEvent_CommandResultACLDenied, onCommandResult); /* Unsubscribe pass NULL callback */ ValkeyModule_SubscribeToServerEvent(ctx, ValkeyModuleEvent_CommandResultFailure, NULL); ``` ### Design Decisions - **Separate events instead of sub-events**: Modules subscribing only to failures have zero overhead for successful commands (~2ns listener-list check vs ~30ns callback invocation per command). This is critical since success events fire on the hot path of every command. - **Stack-allocated info struct**: The `ValkeyModuleCommandResultInfoV1` is built on the stack ΓÇö no heap allocation per event. - **Zero-copy argv**: Arguments are passed directly from the client's argv array. Any integer-encoded arguments (from `tryObjectEncoding()` during command execution) are decoded to string-encoded objects before being passed to the callback, ensuring compatibility with `ValkeyModule_StringPtrLen()`. - **Early exit**: If no modules are subscribed to any server events, the event firing function returns immediately before building the info struct. - **Uses existing server event infrastructure**: Follows the `ValkeyModule_SubscribeToServerEvent()` pattern used by all other server events, rather than introducing a new callback mechanism. ### Files Changed | File | Change | |---|---| | `src/valkeymodule.h` | Event IDs, event constants, `ValkeyModuleCommandResultInfoV1` struct | | `src/module.c` | `moduleFireCommandResultEvent()`, event documentation, event version entries | | `src/module.h` | Function declaration | | `src/server.c` | Call `moduleFireCommandResultEvent()` from `call()` after command execution | | `src/server.c` | Call to `moduleFireCommandACLDeniedEvent` in `processCommand` after ACL rejection | | `tests/modules/commandresult.c` | Test module exercising the full API | | `tests/unit/moduleapi/commandresult.tcl` | Integration tests | --------- Signed-off-by: martinrvisser <mvisser@hotmail.com> Signed-off-by: martinrvisser <martinrvisser@users.noreply.github.com> Co-authored-by: Ricardo Dias <rjd15372@gmail.com>

…unload (#3545) This follows up on the commandresult API work and fixes cleanup around unsubscribe and module unload. The main issue was that command-result event listeners could leave stale state behind. On unload, we removed the listeners themselves but didn’t fully update the fast-path listener counters. Separately, unsubscribing with a NULL callback could behave badly if the listener wasn’t present anymore. In practice, that meant later commands could still walk into command-result event handling after the module was supposed to be cleaned up. Failed in Daily as well yesterday: https://github.com/valkey-io/valkey/actions/runs/24753491944/job/72421581610#step:10:852 Related Failures: #2936 (comment) --------- Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

…sers) Removed valkey-io#3504, valkey-io#3545, valkey-io#3503 - these fix bugs introduced by valkey-io#3324, valkey-io#2936, Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com> valkey-io#3392 respectively, all new in RC2. Users never experienced these bugs.

…unload (#3545) This follows up on the commandresult API work and fixes cleanup around unsubscribe and module unload. The main issue was that command-result event listeners could leave stale state behind. On unload, we removed the listeners themselves but didn’t fully update the fast-path listener counters. Separately, unsubscribing with a NULL callback could behave badly if the listener wasn’t present anymore. In practice, that meant later commands could still walk into command-result event handling after the module was supposed to be cleaned up. Failed in Daily as well yesterday: https://github.com/valkey-io/valkey/actions/runs/24753491944/job/72421581610#step:10:852 Related Failures: valkey-io/valkey#2936 (comment) --------- Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

github-actions Bot assigned martinrvisser Dec 15, 2025

martinrvisser force-pushed the module-command-result branch from 132d2f7 to 094bb30 Compare December 18, 2025 15:16

martinrvisser force-pushed the module-command-result branch 4 times, most recently from 39598c8 to ee2db4a Compare January 8, 2026 15:27

rjd15372 requested changes Jan 12, 2026

View reviewed changes

martinrvisser requested a review from rjd15372 January 15, 2026 11:26

rjd15372 reviewed Jan 15, 2026

View reviewed changes

Comment thread src/module.c Outdated

zuiderkwast reviewed Jan 15, 2026

View reviewed changes

Comment thread src/valkeymodule.h Outdated

martinrvisser requested review from rjd15372 and zuiderkwast January 22, 2026 13:01

PingXie requested review from Copilot and removed request for rjd15372 and zuiderkwast January 26, 2026 16:08

Copilot started reviewing on behalf of PingXie January 26, 2026 16:09 View session

Copilot AI reviewed Jan 26, 2026

View reviewed changes

murphyjacob4 reviewed Jan 26, 2026

View reviewed changes

madolson reviewed Jan 26, 2026

View reviewed changes

martinrvisser force-pushed the module-command-result branch 2 times, most recently from d624e9e to 9d40ccc Compare April 15, 2026 13:55

martinrvisser force-pushed the module-command-result branch from 9d40ccc to 6f0ad81 Compare April 15, 2026 13:58

murphyjacob4 reviewed Apr 15, 2026

View reviewed changes

martinrvisser added 3 commits April 17, 2026 09:44

two high level events ACL rejected and non-ACL rejected, expose full …

b986ef6

…error message for non-ACL Signed-off-by: martinrvisser <mvisser@hotmail.com>

fix for cluster_err_msg

ba0a01a

Signed-off-by: martinrvisser <mvisser@hotmail.com>

fix double string termination

fe126a9

Signed-off-by: martinrvisser <mvisser@hotmail.com>

martinrvisser requested a review from murphyjacob4 April 19, 2026 18:53

murphyjacob4 approved these changes Apr 20, 2026

View reviewed changes

murphyjacob4 added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Apr 20, 2026

removed wrapper functions in processCommand and moved into rejectComm…

2d50f02

…and and rejectCommandSds Signed-off-by: martinrvisser <mvisser@hotmail.com>

lucasyonge merged commit 6444717 into valkey-io:unstable Apr 21, 2026
58 checks passed

github-project-automation Bot moved this from Needs Review to To be backported in Valkey 9.1 Apr 21, 2026

This was referenced Apr 21, 2026

Stabilize diskless no-drop replication test #3511

Merged

Add zmalloc_aligned() and fix SPMC queue buffer alignment #3504

Merged

Fix module commandresult event cleanup during unsubscribe and module unload #3545

Merged

sarthakaggarwal97 mentioned this pull request Apr 23, 2026

Backport Unstable to 9.1 for RC2 #3519

Merged

sarthakaggarwal97 added the release-notes This issue should get a line item in the release notes label Apr 28, 2026

sarthakaggarwal97 mentioned this pull request May 4, 2026

[backport] Weekly backport sweep for 9.1 sarthakaggarwal97/valkey#225

Closed

sarthakaggarwal97 moved this from To be backported to Done in Valkey 9.1 May 16, 2026

		rejectCommandFormat(c, "Command '%s' not allowed inside a transaction", c->cmd->fullname);
		moduleFireCommandRejectedEvent(c, VALKEYMODULE_SUBEVENT_COMMAND_RESULT_REJECTED_OTHER, -1, "NOMULTI");

Uh oh!

Conversation

martinrvisser commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Add Command Result Event Notifications for Modules

Summary

Motivation

API

Events

Event Data: ValkeyModuleCommandResultInfo

Usage Example

Design Decisions

Files Changed

Uh oh!

rjd15372 commented Dec 15, 2025

Uh oh!

codecov Bot commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

martinrvisser commented Dec 15, 2025

Uh oh!

rjd15372 commented Dec 16, 2025

Uh oh!

martinrvisser commented Jan 8, 2026

Uh oh!

rjd15372 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!

Uh oh!

Uh oh!

martinrvisser commented Jan 14, 2026

Uh oh!

Uh oh!

rjd15372 commented Jan 15, 2026

Uh oh!

rjd15372 commented Jan 15, 2026

Uh oh!

Uh oh!

dmitrypol commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinrvisser commented Dec 15, 2025 •

edited

Loading

Event Data: `ValkeyModuleCommandResultInfo`

codecov Bot commented Dec 15, 2025 •

edited

Loading

dmitrypol commented Jan 26, 2026 •

edited

Loading