Skip to content

fix(txn): discard partial writes from failed EXEC commands#3556

Open
advisedy wants to merge 2 commits into
apache:unstablefrom
advisedy:fix-3555-txn-savepoint
Open

fix(txn): discard partial writes from failed EXEC commands#3556
advisedy wants to merge 2 commits into
apache:unstablefrom
advisedy:fix-3555-txn-savepoint

Conversation

@advisedy

Copy link
Copy Markdown

Summary

Fixes #3555.

Add command-level RocksDB savepoints during EXEC so a failed write command
cannot leave partial ops in the shared transaction batch.

Changes

  • Add Storage::{Set,Pop,Rollback}TxnSavePoint around the transaction write batch
  • Apply savepoints in Connection::ExecuteCommands for kCmdWrite during EXEC
  • Apply the same boundary for nested script write commands in scripting.cc
  • Mark EVAL / FCALL with kCmdSkipTxnSavepoint so outer script errors do not
    roll back already successful nested writes
  • On fatal savepoint errors, EXEC aborts the shared batch and returns errors for
    the queued command list

Tests

Go cases in tests/gocase/unit/multi/multi_test.go:

  1. Issue repro: a failed HSET under write_batch_max_bytes leaves no partial
    hash fields; the following SET still commits
  2. An EVAL runtime error keeps a previously successful nested SET
  3. A failed multi-field write via redis.pcall rolls back only that command; a
    later nested SET still commits

@advisedy

Copy link
Copy Markdown
Author

Hi @PragmaTwice, this PR tries to fix #3555 with command-level savepoints during EXEC so a failed write does not leave partial ops in the shared batch.

Would you mind taking a look when you have time?
Thanks!

@advisedy

Copy link
Copy Markdown
Author

@PragmaTwice, Thanks for triggering CI.

The only failure is :

--- FAIL: TestSlaveLostMaster (15.59s)
    client.go:99: forwarding tcp stream stopped
    client.go:147: forward tcp stream failed, err: read tcp 127.0.0.1:38281->127.0.0.1:35050: read: connection reset by peer

This matches the known flake tracked in #3515 and is unrelated to this PR.

The new test in unit/multi passed:

ok  github.com/apache/kvrocks/tests/gocase/unit/multi  0.505s

Could you please re-run the failed jobs when you have a chance?

@PragmaTwice PragmaTwice requested a review from mapleFU July 13, 2026 14:02
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 50%)

See analysis details on SonarQube Cloud

@advisedy

Copy link
Copy Markdown
Author

Hi @mapleFU , quick question — all CI checks pass, and I've added
Go tests for the new behavior (in tests/gocase/unit/multi/multi_test.go).

Does that mean I can ignore the SonarCloud coverage failure here? Thanks!

@mapleFU

mapleFU commented Jul 14, 2026

Copy link
Copy Markdown
Member

I'm quite busy these days, will review before 7.16 night

@PragmaTwice

Copy link
Copy Markdown
Member

Does that mean I can ignore the SonarCloud coverage failure here? Thanks!

SonarCloud failure is not a blocker. Reviewer will decide if it's good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

3 participants