fix(csc): drop module movablekeys extraction (#4212)

Redis resolves the keys it must invalidate in getKeysFromCommand(): the
module getkeys API, then getkeys_proc, then the legacy range. Key specs are
never consulted, so a module command that declares movable keys without
registering a getkeys callback is tracked by its legacy positions alone,
usually none. cscDeriveMeta indexed the spec positions instead, and the
entry then sits stale for its whole life because no invalidation ever
matches it.

Deriving no extraction means the command is never cached, so the guarantee
no longer depends on remembering each module command in a dont_cache
correction. The cacheable-command list is unchanged; the pinned
fingerprint moves because ts.nrange and ts.nrevrange now derive no
extraction.

Refs #4097

Co-authored-by: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com>

csc_commands.go

@@ -195,6 +195,7 @@ func cscDeriveMeta(info *CommandInfo) cscCommandMeta {
 
 	// The readonly FLAG is the normative source; the ReadOnly convenience
 	// bool is deliberately ignored.
+	var moduleCmd, movableKeys bool
 	for _, f := range info.Flags {
 		switch f {
 		case "readonly":
@@ -203,6 +204,10 @@ func cscDeriveMeta(info *CommandInfo) cscCommandMeta {
 			meta.bits |= cscFlagScriptRunner
 		case "blocking":
 			meta.bits |= cscFlagBlocking
+		case "module":
+			moduleCmd = true
+		case "movablekeys":
+			movableKeys = true
 		}
 	}
 	for _, t := range info.Tips {
@@ -235,11 +240,22 @@ func cscDeriveMeta(info *CommandInfo) cscCommandMeta {
 		meta.step = int16(info.StepCount)
 	}
 
-	canExtract := (other == 0 && complete <= 1) || override.extraKeySpecs.permits(info, complete, other)
+	hasOverride := override.extraKeySpecs.permits(info, complete, other)
+	canExtract := (other == 0 && complete <= 1) || hasOverride
 	if !canExtract {
 		return meta
 	}
 
+	// Invalidation follows the positions getKeysFromCommand() reports: the
+	// module getkeys API, then getkeys_proc, then the legacy range; never a key
+	// spec. A module command that declares movable keys may register no getkeys
+	// callback and is then tracked by its legacy positions alone, usually none.
+	// Its spec positions cannot be checked against what the server tracks, so
+	// without an override vouching for the shape it must not be cached.
+	if moduleCmd && movableKeys && !hasOverride {
+		return meta
+	}
+
 	if complete == 1 && completeSpec.BeginSearch == "index" {
 		switch completeSpec.FindKeys {
 		case "range":

csc_test.go

@@ -490,6 +490,66 @@ func TestCSCDeriveMeta(t *testing.T) {
 	}
 }
 
+// Redis invalidates the positions getKeysFromCommand() reports (module getkeys
+// API, getkeys_proc, then the legacy range), never a key spec, so a module
+// command with movable keys and no getkeys callback tracks only its legacy
+// positions. Its spec cannot be trusted to match, and a command whose keys
+// cannot be listed is never cached.
+func TestCSCDeriveMetaModuleMovableKeysFailClosed(t *testing.T) {
+	numkeysSpec := KeySpec{
+		Flags: []string{"RO", "access"}, BeginSearch: "index", Index: 1,
+		FindKeys: "keynum", FirstKey: 1, KeyStep: 1,
+	}
+	rangeSpec := KeySpec{
+		Flags: []string{"RO", "access"}, BeginSearch: "index", Index: 1,
+		FindKeys: "range", KeyStep: 1,
+	}
+
+	variants := []struct {
+		name string
+		info *CommandInfo
+	}{
+		{"keynum", &CommandInfo{
+			Name: "ext.nmrange", Flags: []string{"readonly", "module", "movablekeys"},
+			KeySpecs: []KeySpec{numkeysSpec},
+		}},
+		{"range", &CommandInfo{
+			Name: "ext.nmrange2", Flags: []string{"readonly", "module", "movablekeys"},
+			KeySpecs: []KeySpec{rangeSpec},
+		}},
+	}
+	for _, tc := range variants {
+		t.Run(tc.name, func(t *testing.T) {
+			if m := cscDeriveMeta(tc.info); m.extract != cscKeyExtractNone {
+				t.Errorf("module movablekeys must derive no extraction, got %+v", m)
+			}
+			// The live-record path is the one PreferLive exposes to modules.
+			view := buildCommandMetadataView(map[string]*CommandInfo{tc.info.Name: tc.info}, nil)
+			cmd := makeCmd(tc.info.Name, 1, "s1", "-", "+")
+			if keys := extractRedisKeysInView(view, cmd); len(keys) != 0 {
+				t.Errorf("module movablekeys extracted keys the server cannot invalidate: %v", keys)
+			}
+		})
+	}
+
+	// Controls: the gate is the movable keys flag, not module-ness and not a
+	// runtime key count.
+	fixed := &CommandInfo{
+		Name: "ext.card", Flags: []string{"readonly", "module"},
+		FirstKeyPos: 1, LastKeyPos: 1, StepCount: 1, KeySpecs: []KeySpec{rangeSpec},
+	}
+	if m := cscDeriveMeta(fixed); m.extract != cscKeyExtractRange {
+		t.Errorf("module command with fixed key positions lost extraction: %+v", m)
+	}
+	builtIn := &CommandInfo{
+		Name: "zdiffish", Flags: []string{"readonly", "movablekeys"},
+		KeySpecs: []KeySpec{numkeysSpec},
+	}
+	if m := cscDeriveMeta(builtIn); m.extract != cscKeyExtractKeynum || m.numkeysAt != 1 || m.firstKey != 2 {
+		t.Errorf("built-in movablekeys command lost keynum extraction: %+v", m)
+	}
+}
+
 func TestCommandMetadataViewWithApplicationOverrides(t *testing.T) {
 	// An application override record replaces every lower layer: it can
 	// exclude a cacheable command...
@@ -5429,7 +5489,7 @@ func TestCSCPerformanceCompatibility(t *testing.T) {
 	if got := fmt.Sprintf("%x", sha256.Sum256([]byte(dump))); got != "cd52438234a475701fd546e2a685cf493e7486bd78ebe2bbb8ac7f224485b609" {
 		t.Fatalf("eligible-command dump changed: sha256=%s", got)
 	}
-	if view.cscFingerprint != "0255d4ea837ce42f4814b7b4b9ff820c" {
+	if view.cscFingerprint != "789bd9d45282d1f83111e8ef96eaac1a" {
 		t.Fatalf("default CSC fingerprint changed: %s", view.cscFingerprint)
 	}
 	if path := os.Getenv("CSC_COMPATIBILITY_DUMP"); path != "" {