ZADD NX/XX/GT/LT can diverge on a standby for keys owned by a remote node group (forward image is pre-filter)

CommitZAdd does not evaluate the NX/XX/GT/LT flags — it applies every element it is given (only honoring INCR). All flag filtering happens earlier in ExecuteOn, which removes filtered-out elements from the command in place. For a key owned by a remote node group, the standby-forward path re-serializes the coordinator's original pre-filter command image instead of the owner's post-ExecuteOn command, so the standby applies the unfiltered element set and diverges from the primary.

Evidence

CommitZAdd applies all elements, no NX/XX/GT/LT check (src/redis_zset_object.cpp:494-561): the update_element lambda only branches on params.INCR(); the loops call it for every element unconditionally.

Filtering is done in ExecuteOn (src/redis_zset_object.cpp:355-388), which mutates the command's elements in place (sets filtered entries to monostate).

Standby forward (data_substrate/tx_service/src/standby.cpp StandbyForwardEntry::AddTxCommand):

if (cc_req.IsLocal()) {
    cmd->Serialize(cmd_str);                 // local owner: post-ExecuteOn (filtered) — OK
    req.add_cmd_list(std::move(cmd_str));
} else {
    req.add_cmd_list(*cc_req.CommandImage()); // remote owner: coordinator's PRE-filter image
}

For a remote object command (coordinator NG ≠ owner NG), the owner's standby is forwarded the original unfiltered image, which CommitZAdd then applies in full.

Scenario (cluster mode, multi-NG, with a standby)

Key z owned by NG B; a ZADD z GT 5 m issued via coordinator NG A. On B the score does not satisfy GT and is filtered out, so B's primary is unchanged; but B's standby receives the pre-filter image and applies the update → standby score ≠ primary score. NX/XX/LT behave analogously. After a promotion of B's standby, the divergence becomes the served state.

Fix: forward the post-ExecuteOn (filtered) command image for remote object commands too (or have CommitZAdd re-evaluate the flags on replay/forward).

Found during a code audit (docs PR #492). CommitZAdd ignoring flags and the else-branch pre-filter forward are verified against source; please confirm the WAL image capture point for the same path.

🤖 Found with Claude Code

chore: point data_substrate at merged main (#509)

build: reconcile third-party workspace onto main

Follow-up audit results: (1) the WAL capture point is confirmed to have the same defect, and (2) a survey of all conditional write commands found two more commands with the same hazard pattern (LTRIM, SPOP) plus a related WAL gap in the EXPIRE family. Details below.

1. WAL confirmed: remote-owned keys log the pre-`ExecuteOn` image too

The redo-log image of an object command is serialized from the coordinator's own command object at commit time: tx_execution.cpp:6848 passes obj_cmd_op.command_ into cmd_set_.AddObjectCommand(...), which calls cmd->Serialize(cmd_str) in CmdSetEntry::AddCommand (data_substrate/tx_service/include/read_write_entry.h:182); the per-NG log blob is assembled from cmd_str_list_ (tx_execution.cpp:4796-4830).

Local owner: the coordinator's command object is the very object ExecuteOn mutated in place (local_cc_handler.cpp:1721-1736, object_cc_map.h:1071) → post-filter image. Correct.
Remote owner: ExecuteOn ran on the owner's parsed copy (object_cc_map.h:199-202); the coordinator's object was never executed → the WAL gets the pre-filter image.

Log replay is commit-only, exactly like standby apply: ReplayLogCc reconstructs commands and applies them via EmplaceAndCommitBufferedTxnCommand → cmd->CommitOn(...) (object_cc_map.h:2452, cc_entry.h:1822); standby apply is CreateTxCommand(cmd_str) → CommitOn (object_cc_map.h:1983-1991). ExecuteOn is never re-run on either path, so flags are never re-evaluated.

Consequence: for remote-owned keys the divergence is not limited to standbys — crash recovery replays the unfiltered command on the primary itself.

2. LTRIM: same pattern, worse consequences (full list wipe / iterator UB)

RedisListObject::Execute(LTrimCommand &) normalizes negative / out-of-range start_/end_ in place against the current list size (src/redis_list_object.cpp:275-305), and Serialize writes the mutated values (src/redis_command.cpp:4509-4510). CommitLTrim assumes already-normalized non-negative indices and has no negative handling (src/redis_list_object.cpp:827-853).

Replaying a pre-ExecuteOn image through commit-only:

LTRIM key 0 -1 (the common "keep everything" idiom): CommitLTrim(0, -1) hits end + 1 (== 0) < size and erases from begin() to end() — the replica wipes the entire list while the primary keeps all elements.
More negative end (e.g. -5): list_object_.begin() + end + 1 is iterator arithmetic with a negative offset — undefined behavior, potential crash/corruption.

Affects both the standby forward and the WAL for remote-owned keys.

3. SPOP: standby diverges; the WAL is correct only by accident

SPOP is random, so replay cannot recompute the selection: by design SPopCommand::Serialize writes the popped members from result_.string_list_ (src/redis_command.cpp:7176-7193; the comment at :7182-7184 says the log must bookkeep popped members "for idempotence"), and CommitSPop removes exactly the serialized members.

Standby, remote owner: the forwarded image is the coordinator's original bytes with an empty member list → CommitSPop removes nothing → primary popped N members, standby popped none.
WAL, remote owner: happens to be correct — the popped members round-trip back to the coordinator inside the command result (RedisHashSetResult::Serialize/Deserialize includes string_list_, src/redis_command.cpp:1130-1152; deserialized into the coordinator's result_ from ApplyResponse.cmd_result at data_substrate/tx_service/src/remote/cc_stream_receiver.cpp:1875-1888), so the coordinator's commit-time Serialize emits the populated list. Fragile (correctness depends on the result payload doubling as the effect image), but currently correct.

4. EXPIRE family: flags are safe, but the `RecoverObjectCommand` snapshot never reaches the WAL for remote-owned keys

EXPIRE/PEXPIRE/EXPIREAT/GETEX/PERSIST evaluate NX/XX/GT/LT into a local (will_set_ttl, src/redis_command.cpp:7823-7880) and never mutate the command image, so they don't have the ZADD-style pre/post asymmetry. But they rely on a companion mechanism: when a TTL will actually be reset, ExecuteOn snapshots the whole object into recover_ttl_obj_cmd_ (src/redis_command.cpp:7933-7934; GETEX :8230-8231; PERSIST :7749-7750) so that replay does not depend on a possibly-gone base object, and the engine logs that snapshot instead (tx_execution.cpp:6821-6841, RecoverTTLObjectCommand()).

Standby: OK even for remote owners — the owner-side forward adds the snapshot directly (object_cc_map.h:1080-1086, AddOverWriteCommand(recover_command)).
WAL: broken for remote owners — the logging branch is guarded by cmd_result.ttl_reset_ (tx_execution.cpp:6792), but ApplyResponse does not carry ttl_reset (data_substrate/tx_service/include/proto/cc_request.proto:1353-1364) and the coordinator only back-fills rec_status / cmd_result / commit_ts / last_vali_ts / lock_type / object_modified (cc_stream_receiver.cpp:1887). So the snapshot branch never fires for a remote-owned key — the WAL only holds the bare EXPIRE image. Even if the flag were propagated, RecoverTTLObjectCommand() would hit assert(recover_ttl_obj_cmd_ != nullptr) on the coordinator, because the snapshot only exists on the owner's parsed copy.

5. Survey of the remaining conditional commands — no other instances found

Key-level gates via ProceedOnExistentObject() / ProceedOnNonExistentObject() (SET NX/XX include/redis_command.h:1435-1451, LPUSHX/RPUSHX, EXPIRE/PERSIST existence): decided before execution, all-or-nothing, image unmutated — safe.
Execute-time all-or-nothing gates (HSETNX field-level NX src/redis_hash_object.cpp:423, LINSERT pivot check, MSETNX two-phase exists-check): the decision doesn't mutate the image; a filtered command is simply not committed/forwarded — safe.
Commit recomputes from state (SADD/SREM/HSET/HDEL/INCR*/APPEND/SETRANGE/SETBIT/BITFIELD/HINCRBY*/LPUSH/LPOP/LREM/ZREM/ZREMRANGEBY*/ZPOPMIN/ZPOPMAX): deterministic replay from unmutated args — safe.
SMOVE: the dest SADD's args are populated by HandleMiddleResult on the coordinator before step-1 dispatch (src/redis_command.cpp:6910-6926), so local and remote images already contain the decision — safe.
LSET: mutates index_ in Execute, but CommitLSet defensively re-normalizes negatives (src/redis_list_object.cpp:672-677) — safe, and a model for fixing LTRIM on the commit side.
GETRANGE/BITPOS/LINDEX mutate command fields but are read-only — never persisted/forwarded.

Fix implications

The owner-side one-line fix suggested in the report — serialize the executed parsed copy in the else branch of StandbyForwardEntry::AddTxCommand (cmd->Serialize() instead of *cc_req.CommandImage()) — repairs the standby path for ZADD, LTRIM and SPOP at once.
The WAL path needs a separate fix, since its image is serialized from the coordinator's never-executed object. Options: ship the post-ExecuteOn image back in ApplyResponse (which would also fix the EXPIRE-family snapshot gap in §4), or make commit deterministic per command (re-evaluate NX/XX/GT/LT in CommitZAdd; re-normalize indices in CommitLTrim the way CommitLSet does). SPOP's randomness means it can only be fixed by an image round-trip — which it already implicitly has for the WAL via its result payload.

Follow-up to the original audit (docs PR #492). Engine paths are in the data_substrate submodule (eloqdata/tx_service); all cited code verified against current main.

🤖 Found with Claude Code

Fixes are up, split across the two repos:

fix: remote-owned keys forwarded/logged pre-ExecuteOn command images (eloqkv#509) tx_service#523 — (a) StandbyForwardEntry::AddTxCommand serializes the owner-side executed command for both local and remote ApplyCc (fixes the standby divergence for ZADD/LTRIM/SPOP reported here); (b) ApplyResponse gains ttl_reset/recover_cmd_image/ttl/ttl_expired (fields 10-13) so remote-owned keys WAL-log the EXPIRE-family snapshot, the post-command validity horizon, and the expired-recreation retire record.
fix: make ZADD/LTRIM commit deterministic from state for replay (#509) #533 (depends on fix: size redis command round counters by enum range #523) — CommitZAdd re-applies NX/XX/GT/LT via the same filter functions as ExecuteOn, and CommitLTrim normalizes indices + clears empty ranges before any iterator arithmetic — ZADD/LTRIM replay is now deterministic from a pre-ExecuteOn image, covering the WAL/recovery path with no protocol dependency.

Test coverage (all red-first — every case reproduced the bug on the pre-fix code before the fix turned it green):

StandbyForward-Test (engine): remote forward carried the pre-image ("PRE" == "POST" failure pre-fix).
ApplyResponseBoundary-Test (engine): fill → real protobuf wire → resend equivalence → back-fill → CmdSetEntry WAL-input legs; pre-fix reds: snapshot dropped, finite ttl dropped, expired-recreation ttl rule.
command_replay_test (eloqkv, -DBUILD_WITH_TESTS=ON): differential primary-vs-commit-only-replica simulation; 10 red cases pre-fix — 6 assertion divergences and 4 crashes (the LTRIM empty-range cases are UB on the old code).
Regression: engine ctest 52/52, eloqkv unit tests 28/28, TCL zset/list/expire/set green.

Two pre-existing bugs found and fixed along the way (both reachable WITHOUT the remote path):

LTRIM key 10 -2 on a 5-element list was out-of-bounds iterator arithmetic on a purely local key (Execute normalizes to (5,3), engine unconditionally calls CommitOn, CommitLTrim truncates then computes begin()+5). Fixed by the commit-side empty-range guard (eloqkv#533).
Recovery discarded acknowledged writes that recreated an expired key, local keys included: the owner reported the pre-command (expired) ttl, the WAL record's validity horizon was in the past, and ReplayLogCc treats that as "object deleted". Fixed by ComputeReportedTtl reporting the post-command ttl (tx_service#523).

Rolling upgrade: proto3-additive, no wire break; mixed-version clusters keep today's behavior on un-upgraded paths until both sides run the fix (details + release-note text in the PRs).

Follow-ups (not in these PRs):

The multi-object command post-process path has no ttl_reset snapshot logging (same class as fixed here for single-object; theoretical today — no current multi-object command both resets a TTL and is non-overwrite).
No e2e harness covers remote-owner + standby + WAL-replay scenarios (TestCluster: record-only workload, no standbys, skip_wal recovery). Extending it would allow true end-to-end regression tests for all three fixes.

🤖 Generated with Claude Code

ci: run the BUILD_WITH_TESTS unit tests after the build

fix: standby forward serializes the executed command for remote ApplyCc

fix: propagate owner TTL facts in ApplyResponse and fix expired-recre…

docs: reflect #509 standby-forward and ApplyResponse TTL fixes

fix: remote-owned keys forwarded/logged pre-ExecuteOn command images …

chore: bump data_substrate for #509 engine fixes (tx_service#523)

fix: make object command replay deterministic (eloqdata#509)

Navigation Menu

Repository navigation

ZADD NX/XX/GT/LT can diverge on a standby for keys owned by a remote node group (forward image is pre-filter) #509

Evidence

Scenario (cluster mode, multi-NG, with a standby)

1. WAL confirmed: remote-owned keys log the pre-`ExecuteOn` image too

2. LTRIM: same pattern, worse consequences (full list wipe / iterator UB)

3. SPOP: standby diverges; the WAL is correct only by accident

4. EXPIRE family: flags are safe, but the `RecoverObjectCommand` snapshot never reaches the WAL for remote-owned keys

5. Survey of the remaining conditional commands — no other instances found

Fix implications

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

ZADD NX/XX/GT/LT can diverge on a standby for keys owned by a remote node group (forward image is pre-filter) #509

Description

Evidence

Scenario (cluster mode, multi-NG, with a standby)

Activity

liunyl commented on Jul 11, 2026

1. WAL confirmed: remote-owned keys log the pre-ExecuteOn image too

2. LTRIM: same pattern, worse consequences (full list wipe / iterator UB)

3. SPOP: standby diverges; the WAL is correct only by accident

4. EXPIRE family: flags are safe, but the RecoverObjectCommand snapshot never reaches the WAL for remote-owned keys

5. Survey of the remaining conditional commands — no other instances found

Fix implications

liunyl commented on Jul 11, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

1. WAL confirmed: remote-owned keys log the pre-`ExecuteOn` image too

4. EXPIRE family: flags are safe, but the `RecoverObjectCommand` snapshot never reaches the WAL for remote-owned keys