data loss: every two-key write whose destination is not the routing key lands on the wrong shard (SMOVE, RENAME, *STORE)

[TinDang97]

Found while fixing #570 (PR #591). #570 was scoped to BLMOVE/BRPOPLPUSH; the same routing rule breaks every two-key write whose destination is not the routing key, and #591 deliberately fixed only the LMOVE family.

Mechanism

A keyed command is routed by extract_primary_key, which returns args[0]. The owning shard of that ONE key then executes the whole command against its own slice. For a command that also WRITES a second key, the write lands in the routing key's shard under the second key's name — invisible to every normally-routed read of that name, which goes to its owner.

Measured

Merge-base binary fc60836c, --shards 4, 12 key placements each, FLUSHALL between trials. "LOST" = the command reported success and the destination is not visible afterwards.

command	dst visible	refused	LOST	last reply
RPOPLPUSH a b	1	0	11	m (element gone from both keys)
SMOVE a b m	1	0	11	1 (member gone from both sets)
RENAME a b	1	0	11	+OK (key gone from both names)
SINTERSTORE b a	1	0	11	0 (source intact, dest written to wrong shard)
ZRANGESTORE b a 0 -1	1	0	11	0 (source intact, dest written to wrong shard)

RPOPLPUSH is fixed by #591. The remaining four are not.

Two distinct severities:

• Acked data loss — SMOVE, RENAME. The value leaves the source and is not readable at the destination. RENAME is the sharpest: +OK, and the key exists under neither name.
• Wrong result, no loss — SINTERSTORE, ZRANGESTORE. The source survives; the destination is written to the wrong shard's table, so a later read sees nothing and the count returned is computed from a source the executing shard could not see either.

Not exhaustive. The same shape is expected for at least RENAMENX, SUNIONSTORE, SDIFFSTORE, ZUNIONSTORE, ZINTERSTORE, ZDIFFSTORE, GEORADIUS ... STORE/STOREDIST, GEOSEARCHSTORE, SORT ... STORE, BITFIELD-adjacent stores and LCS ... IDX-adjacent forms — every one of these should be measured, not assumed.

COPY is NOT affected: it is in is_multi_key_command and coordinated by coordinate_copy, which hops to the destination's owner (degrading to an error for non-string values). BITOP, MSET, MSETNX, MGET, DEL/UNLINK/EXISTS are likewise coordinated.

Suggested direction

A per-command guard for each of these repeats #591's work N times. The systemic version is one rule at the routing choke point:

if a command is a WRITE, is not in is_multi_key_command (the set that IS coordinated), and its declared keys span more than one shard, refuse CROSSSLOT.

analyze_txn_locality already implements exactly this test for MULTI/EXEC bodies, including the positional STORE/STOREDIST destinations that command_keys does not cover, and #583 landed a shared key-position walker with acl::keyspec that understands every one of these layouts. The pieces exist; what is missing is applying them to single commands.

Caveats for whoever picks this up:

• the guard must be behind the is_multi_key_command allow-list, or it will break MSET/DEL/MGET, which legitimately span shards;
• movablekeys commands (SORT ... STORE, GEORADIUS ... STORE, EVAL) are not covered by the fixed first_key/last_key/step specs — store_clause_dest in shared.rs is the existing precedent;
• it must be applied on BOTH dispatch paths (handler_monoio and handler_sharded), or it is invisible on the shipped runtime;
• tests should assert conservation (exactly one copy of the value exists, and the reply agrees with where it is) rather than the error string, so the suite survives a future cross-shard hop — see tests/list_move_cross_shard.rs.

Repro script and the raw table above are reproducible with a 4-shard server and the five commands listed; each needs --disk-free-min-pct 0 on a near-full data volume.

[TinDang97]

Independently reproduced — RENAME loses data 8 times out of 12 at --shards 4

Verified on a binary built fresh from 5f0cc0a8 (main + two unrelated vector/persistence commits; neither touches RENAME or routing). Raw sockets, no client library.

Minimal case — first attempt, ordinary key names, no crafting required:

SET alpha VALUE-1      -> +OK
GET alpha              -> "VALUE-1"
RENAME alpha omega     -> +OK        <-- claims success
GET alpha              -> nil        <-- source destroyed
GET omega              -> nil        <-- destination never written
EXISTS alpha omega     -> 0          <-- the data is simply gone

Shard-count control, 12 untagged pairs each, identical script:

shards	RENAME acked +OK, destination EMPTY
1	0 / 12
4	8 / 12

So it is purely a routing defect, and at 4 shards roughly ⅔ of all RENAMEs on untagged keys destroy the value.

Why this deserves P0 rather than "follow-up"

• RENAME is a single-key-shaped command to every user — there is no numkeys, no STORE clause, nothing that hints a client should co-locate the two names. Nobody hash-tags a RENAME.
• It is the standard primitive for atomic swap-in (RENAME staging:x live:x), which means the failure lands on exactly the data a user was most careful about.
• The server acks +OK. There is no error, no log line, no metric. A client cannot detect it, and a backup taken afterwards captures the absence.
• It is silent and permanent: unlike the BLMOVE case in data loss: BLMOVE/BRPOPLPUSH to a remote-owned destination acks the element then discards it #570, there is no blocked client to notice a missing element.

This is not a degraded-performance issue or a parity nit — it is unannounced destruction of committed data on a default-ish configuration.

Suggested handling

Same remedy as #570 and the sharded MULTI/EXEC precedent: reject cross-shard two-key writes loudly (CROSSSLOT-style) rather than routing them to the wrong owner, then implement real owner-routing where semantics allow. Turning silent loss into a visible error is strictly an improvement even before routing lands.

Measurement script and the --shards 1 control are reproducible from the above; happy to attach the harness.