fix: make ZADD/LTRIM commit deterministic from state for replay (#509) - #533
Conversation
Standby apply and WAL recovery replay a command image via Deserialize + CommitOn only, never ExecuteOn. For a remote-owned key the replayed image is the PRE-ExecuteOn one, so the mutations ExecuteOn makes to the command are lost at commit time: - ZADD's NX/XX/GT/LT filtering (Execute mutates elements_) is dropped, so a replica applied filtered-out elements. - LTRIM's index normalization (Execute mutates start_/end_) is dropped, so CommitLTrim misread raw values: (0,-1) wiped the whole list and a deeply negative end did begin()-1 / past-end iterator arithmetic (UB). Fix B1: CommitZAdd re-runs the same NX/XX/GT/LT filter as the Execute dispatch (same predicates, same LT->GT->NX->XX precedence, reusing the existing const ZAdd* helpers) whenever the vector alternative is held and !INCR. A post-ExecuteOn image passes through unchanged (idempotent); the INCR/pair path is untouched (a filtered INCR returns NoChange and never reaches commit). Fix B2: extract LTRIM index normalization into RedisListObject::NormalizeLTrimRange, shared by Execute and CommitLTrim. CommitLTrim now normalizes first and clears the list on an empty range before any iterator arithmetic. This also fixes a pre-existing purely local UB: LTRIM key 10 -2 on a 5-element list normalizes in place to (5,3) and returns ModifiedToEmpty, but the engine still calls CommitOn(5,3) unconditionally, which erased index 4 and then computed begin()+5 out of bounds. Adds tests/unit/eloq/command_replay_test.cpp (new Catch2 binary) modeling the commit-only replay path. Written red-first: the ZADD flag cases failed by assertion and the LTRIM range/local cases crashed (SIGSEGV/SIGABRT) on the unfixed code; all 17 cases pass after the fix. serialized_length_ bookkeeping and the should_not_move_string clone-vs-move behavior for surviving elements are preserved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The ZADD/LTRIM commit-determinism fix (68e06f8) and the engine standby- forward / ApplyResponse TTL fixes altered behavior documented in docs/03-data-model.md; this pays the docs-maintenance debt (per CLAUDE.md): - §4 Serialization: standby forward now serializes the owner-side executed command for both local and remote owners (ExecuteOn may mutate the command; standby applies commit-only). - §6 TTL WAL correctness: remote-owned keys round-trip the snapshot image, post-command ttl, and ttl_expired via ApplyResponse fields 10-13; owner reports POST-command ttl (expired-recreation reports UINT64_MAX). - §9 invariants: new replay-determinism invariant for self-mutating commands (ZADD filters / LTRIM NormalizeLTrimRange+empty-range guard / LSET re-normalization, vs SPOP's result-carrying Serialize). Refs #509 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughReplay handling now makes LTRIM normalization and ZADD predicate filtering deterministic during commit-only recovery. Tests cover pre-image and post-image replay, CTest runs during builds, and documentation records serialization and WAL invariants. ChangesReplay determinism
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Primary
participant CommandImage
participant Replay
participant RedisObject
Primary->>RedisObject: ExecuteOn command
RedisObject-->>CommandImage: Serialize command image
CommandImage->>Replay: Deserialize image
Replay->>RedisObject: CommitOn without ExecuteOn
RedisObject-->>Replay: Replayed final state
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| #define CATCH_CONFIG_MAIN | ||
|
|
||
| #include <catch2/catch_all.hpp> | ||
| #include <cstdint> |
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Found C++ system header after other header. Should be: command_replay_test.h, c system, c++ system, other. [build/include_order] [4]
|
|
||
| #include <catch2/catch_all.hpp> | ||
| #include <cstdint> | ||
| #include <map> |
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Found C++ system header after other header. Should be: command_replay_test.h, c system, c++ system, other. [build/include_order] [4]
| #include <catch2/catch_all.hpp> | ||
| #include <cstdint> | ||
| #include <map> | ||
| #include <string> |
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Found C++ system header after other header. Should be: command_replay_test.h, c system, c++ system, other. [build/include_order] [4]
| #include <cstdint> | ||
| #include <map> | ||
| #include <string> | ||
| #include <string_view> |
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Found C++ system header after other header. Should be: command_replay_test.h, c system, c++ system, other. [build/include_order] [4]
| #include <map> | ||
| #include <string> | ||
| #include <string_view> | ||
| #include <utility> |
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Found C++ system header after other header. Should be: command_replay_test.h, c system, c++ system, other. [build/include_order] [4]
| #include <string> | ||
| #include <string_view> | ||
| #include <utility> | ||
| #include <variant> |
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Found C++ system header after other header. Should be: command_replay_test.h, c system, c++ system, other. [build/include_order] [4]
| #include <string_view> | ||
| #include <utility> | ||
| #include <variant> | ||
| #include <vector> |
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Found C++ system header after other header. Should be: command_replay_test.h, c system, c++ system, other. [build/include_order] [4]
The Catch2 unit binaries (object_serialize_deserialize_test, command_replay_test) were compiled by CI (-DBUILD_WITH_TESTS=ON) but never executed; the replay-determinism regressions for #509 only run there. Add a ctest step after the build (sub-second). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Standby forward serializes the executed command; ApplyResponse carries ttl_reset/recover_cmd_image/ttl/ttl_expired; owner reports post-command ttl. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 participants
Summary
Fix replay divergence for commands whose
ExecuteOnmutates command arguments. Standby apply and WAL recovery useDeserialize+CommitOnwithout rerunningExecuteOn, so commit must be deterministic from either pre- or post-execution command images.Changes
CommitZAddusing the existing filter helpers.BUILD_WITH_TESTSbinaries through CTest in CI.docs/03-data-model.mdfor replay and TTL invariants.CLAUDE.md: squash merge with a rewritten final message; merge submodule PRs before updating and merging the parent repository.data_substrateto merge commit9cd8840d71fece0ef61de85c51050f7355411fdefrom merged engine PR fix: remote-owned keys forwarded/logged pre-ExecuteOn command images (eloqkv#509) tx_service#523.Validation
command_replay_test: 17/17 cases pass on the feature branch.object_serialize_deserialize_test: 11/11 cases pass on the feature branch.tx_servicetarget branch.Fixes #509.
Summary by CodeRabbit
Bug Fixes
LTRIMnormalization, including empty-range behavior.INCRZADDduring replay (NX/XX,GT/LT), aligning results with primary execution.Documentation
Tests
ZADD,LTRIM, andSPOP.ctestwhen enabled.