apache / apache/kvrocks

MULTI/EXEC can commit partial writes from a command that returns a runtime error

Open
#3,555 0 comments 1 reaction 0 assignees View on GitHub
bug
Dominant language
C++
Stars
4.4k
Forks
658
Avg merge
1d 20h
Merged PRs (30d)
10

Description

### 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. An ordinary atomic command such as HSET must not leave only some of its field updates behind. 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 likely needs savepoints at ordinary atomic command execution boundaries while the shared transaction batch is active: establish a savepoint before the command, `PopSavePoint()` after full success, and `RollbackToSavePoint()` on 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.

This must not become an unconditional rollback around every outer command. Redis deliberately retains successful writes performed by an EVAL/Function before a later script runtime error. Nested `redis.call` commands still need their own atomic boundaries, but an outer script/function error must not roll back its already successful nested writes. For example, `MULTI; EVAL "redis.call('SET',KEYS[1],'v'); error('boom')" 1 scriptkey; EXEC` should return a script error while leaving `scriptkey=v`.

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 atomic command survives. They should also protect the EVAL/Function behavior above so a command-level fix does not accidentally add Redis-incompatible script rollback.

### Are you willing to submit a PR?

- [ ] I'm willing to submit a PR!

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with Storage::BeginTxn and Storage::GetWriteBatchBase in src/storage/storage.cc, then trace CommandExec in src/commands/cmd_txn.cc and the existing transaction tests. Reproduce the mid-command MemoryLimit case and define command-level rollback boundaries that preserve successful commands. Done means failed atomic commands leave no writes while EVAL/Function retains Redis-compatible script behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, redis
Domain
backend, databases
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.