Make LMOVEM notifications consistent with LMOVE

Project: redis/redis | Language: C | Concepts: api-design, testing-strategy, refactoring, consistency-models | Difficulty: mid
Link: https://github.com/redis/redis/pull/15587

The problem

The LMOVEM command in Redis emitted keyspace notifications in different orders depending on whether the source and destination keys were the same or different. For same-key moves, it emitted a pop notification before a push; for distinct-key moves, it emitted push before pop (matching the older LMOVE command). This inconsistency could confuse users and break assumptions for clients listening to these notifications.

Why it's non-trivial

A naive fix would just swap notification order in one code path, but that could:

• Break compatibility with existing clients if not carefully aligned with both documentation and related commands (LMOVE).
• Miss updating tests, which could let regressions slip through.
• Overlook the subtle difference between same-key and distinct-key moves, which have different side effects and notification semantics.

The approach

Code changes

The core of the fix is to make the notification order for same-key moves match the distinct-key path and LMOVE (push notification before pop notification):

-        notifyKeyspaceEvent(NOTIFY_LIST, wherefrom == LIST_HEAD ? "lpop" : "rpop",
-                            srckey, c->db->id);
         notifyKeyspaceEvent(NOTIFY_LIST, whereto == LIST_HEAD ? "lpush" : "rpush",
                             srckey, c->db->id);
+        notifyKeyspaceEvent(NOTIFY_LIST, wherefrom == LIST_HEAD ? "lpop" : "rpop",
+                            srckey, c->db->id);

What this does:
Moves the push (lpush/rpush) notification before the pop (lpop/rpop) notification for same-key moves. This matches both the distinct-key branch and the LMOVE command, ensuring consistent event order.

Test changes

Regression tests were added to verify notification order for all relevant scenarios:

    test {LMOVEM notifications match LMOVE push-before-pop order} {
        ...
        assert_equal {a} [r lmovem k{t} k{t} left right]
        assert_match "pmessage $pattern __keyevent@*__:rpush k{t}" [$rd read]
        assert_match "pmessage $pattern __keyevent@*__:lpop k{t}" [$rd read]
        ...
        assert_equal {a} [r lmovem src{t} dst{t} left right]
        assert_match "pmessage $pattern __keyevent@*__:rpush dst{t}" [$rd read]
        assert_match "pmessage $pattern __keyevent@*__:lpop src{t}" [$rd read]
        ...
    }

What this does:
Verifies that for both same-key and distinct-key moves, the push notification always precedes the pop notification, matching the intended and documented order.

From the review thread

oranagra:
ohh, i now i see.. so if LMOVM is inconsistent with itself, i think we should fix it.
it makes no sense that one path in the command will emit it one one order and another path in a different order.
considering the case of same key is relatively rare, it could maybe cause bugs in the application (if anyone listens for these).

i see LMOVEM is very recently introduced and not part of any official release yet @sundb please ack?
in that case i think we can afford to modify it to be in line with the docs so that if we some day fix LMOVE we'll have one less command to break.

oranagra:
ok. i'm good with changing the code of LMOVEM to be consistent with LMOVE too.
the most important part is that it'll be consistent with itself (two different modes), and to do that early to avoid breaking too many users.

previously i wanted to change it to be consistent with the docs, and so if one day we change LMOVE (breaking change), we won't also have to break LMOVEM, but that's not that important.

so let's fix the one path that differs in ordering to be consistent with LMOVE.

Takeaways

• APIs should emit consistent side effects for all code paths: Inconsistent notification order for different input cases (same-key vs. distinct-key) can break client expectations and make APIs harder to use correctly.
• Align new features with established behaviors unless there's a strong reason not to: Matching LMOVE's notification order in LMOVEM avoids surprising users and reduces maintenance burden.
• Regression tests should explicitly cover edge cases and event ordering: Automated tests that verify not just the result but also the sequence of side effects (like notifications) are crucial for preventing subtle bugs in observable behavior.