PragmaTwice opened a new issue, #3555: URL: https://github.com/apache/kvrocks/issues/3555
### Search before asking - [x] I had searched in the [issues](https://github.com/apache/kvrocks/issues) and found no similar issues. ### Version Unstable `567fcf6c26fc38367c1bda81ca0541c6ab861086` on Linux, using the default storage encoding. The reproduction only changes `rocksdb.write_options.write_batch_max_bytes` at runtime. ### Minimal reproduce step The following was reproduced four times on the current unstable build. The value `180` makes the first internal field write fit in the transaction batch while the next one reaches the batch limit. It may need a small adjustment if the storage encoding or namespace configuration differs. ```text CONFIG SET rocksdb.write_options.write_batch_max_bytes 0 DEL txhash HSET txhash f1 old1 f2 old2 f3 old3 CONFIG SET rocksdb.write_options.write_batch_max_bytes 180 # Standalone control: this fails atomically and leaves every field unchanged. HSET txhash f1 AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA f2 BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB f3 CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC HMGET txhash f1 f2 f3 # The same HSET inside MULTI leaks one of its writes into the shared batch. MULTI HSET txhash f1 AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA f2 BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB f3 CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC SET after ok HMGET txhash f1 f2 f3 EXEC CONFIG SET rocksdb.write_options.write_batch_max_bytes 0 HMGET txhash f1 f2 f3 GET after ``` The standalone HSET returns: ```text ERR Operation aborted: Memory limit reached ``` and its following HMGET returns `old1`, `old2`, `old3`. ### What did you expect to see? Runtime errors should not abort the remaining queued commands or roll back earlier successful commands, but the failed command itself should leave no partial state. EXEC should therefore return the HSET error, commit `SET after ok`, and both HMGET calls should return `old1`, `old2`, `old3`. ### What did you see instead? EXEC returns: ```text 1) ERR Operation aborted: Memory limit reached 2) OK 3) old1, old2, CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC ``` After EXEC, `GET after` returns `ok`, but HMGET still returns `old1`, `old2`, and the new `f3` value. The failed HSET processes fields in reverse order: its first Put succeeds, the next Put returns `MemoryLimit`, and the successful Put remains in the transaction-wide batch. ### Anything Else? This is a transaction-wide issue rather than an HSET-specific one: - [`Storage::BeginTxn`](https://github.com/apache/kvrocks/blob/567fcf6c26fc38367c1bda81ca0541c6ab861086/src/storage/storage.cc#L972-L984) creates one shared `WriteBatchWithIndex` for the whole EXEC. - [`Storage::GetWriteBatchBase`](https://github.com/apache/kvrocks/blob/567fcf6c26fc38367c1bda81ca0541c6ab861086/src/storage/storage.cc#L1002-L1008) returns an observer to that shared batch in transaction mode, and [`Storage::Write`](https://github.com/apache/kvrocks/blob/567fcf6c26fc38367c1bda81ca0541c6ab861086/src/storage/storage.cc#L714-L720) returns OK without flushing it. - A RocksDB batch operation that exceeds `max_bytes` rolls back only that individual operation. Earlier successful operations from the same Redis command remain in the shared batch. - [`CommandExec`](https://github.com/apache/kvrocks/blob/567fcf6c26fc38367c1bda81ca0541c6ab861086/src/commands/cmd_txn.cc#L83-L90) correctly continues after runtime command errors, but it then commits those leftover operations because there is no per-command rollback boundary. Many multi-record write paths are affected, including MSET/MSETEX/MSETNX, SADD/SREM, ZADD and range removals, list push/pop/trim, XADD/XTRIM, hash mutations, bitmap mutations, and other types. Depending on where the failure occurs, this can leave metadata inconsistent with subkeys, break ZSet's two-column-family index, or commit deletes from a failed trim/pop operation. `write_batch_max_bytes` is a deterministic trigger, but RocksDB read/write errors or decode/validation failures after earlier batch mutations have the same risk. This is distinct from #2992, which handled errors from the final DB commit, and from #2554, which tracks transaction concurrency. The transaction-wide shared batch introduced for #1281 provides EXEC-level atomic commit, but it currently has no command-level savepoints. A general fix would establish a savepoint on the shared `WriteBatchWithIndex` before each queued command, `PopSavePoint()` after full success, and `RollbackToSavePoint()` on any runtime error. The boundary should cover `PutLogData` and associated index updates while preserving writes from previously successful commands. If rollback itself fails, the whole transaction batch should be discarded rather than committed. Regression tests should force a mid-command `MemoryLimit`, verify that earlier and later successful commands still commit, and verify that no value, metadata, secondary-index, or replication-log record from the failed command survives. ### Are you willing to submit a PR? - [ ] I'm willing to submit a PR! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
