From dea4eb3e9189502ddc0204e5ba0fc1e6760397ba Mon Sep 17 00:00:00 2001
From: Shyam Jeedigunta <jeedigv@amazon.com>
Date: Mon, 28 Oct 2024 12:15:23 -0700
Subject: [PATCH] [3.5] Fix risk of a partial write txn being applied

Signed-off-by: Shyam Jeedigunta <jeedigv@amazon.com>
---
 server/etcdserver/apply.go      |  9 +++++----
 server/etcdserver/apply_test.go | 31 +++++++++++++++++++++++++++++--
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/server/etcdserver/apply.go b/server/etcdserver/apply.go
index 2ef151259c1..9a3f18d0f41 100644
--- a/server/etcdserver/apply.go
+++ b/server/etcdserver/apply.go
@@ -478,10 +478,11 @@ func (a *applierV3backend) Txn(ctx context.Context, rt *pb.TxnRequest) (*pb.TxnR
 	_, err := a.applyTxn(ctx, txn, rt, txnPath, txnResp)
 	if err != nil {
 		if isWrite {
-			// end txn to release locks before panic
-			txn.End()
-			// When txn with write operations starts it has to be successful
-			// We don't have a way to recover state in case of write failure
+			// CAUTION: When a txn performing write operations starts, we always expect it to be successful.
+			// If a write failure is seen we SHOULD NOT try to recover the server, but crash with a panic to make the failure explicit.
+			// Trying to silently recover (e.g by ignoring the failed txn or calling txn.End() early) poses serious risks:
+			// - violation of transaction atomicity if some write operations have been partially executed
+			// - data inconsistency across different etcd members if they applied the txn asymmetrically
 			lg.Panic("unexpected error during txn with writes", zap.Error(err))
 		} else {
 			lg.Error("unexpected error during readonly txn", zap.Error(err))
diff --git a/server/etcdserver/apply_test.go b/server/etcdserver/apply_test.go
index 65704fd4e78..b968b89fea3 100644
--- a/server/etcdserver/apply_test.go
+++ b/server/etcdserver/apply_test.go
@@ -2,6 +2,10 @@ package etcdserver
 
 import (
 	"context"
+	"crypto/sha256"
+	"github.com/stretchr/testify/require"
+	"io"
+	"os"
 	"strings"
 	"sync"
 	"testing"
@@ -55,8 +59,7 @@ func TestReadonlyTxnError(t *testing.T) {
 }
 
 func TestWriteTxnPanic(t *testing.T) {
-	b, _ := betesting.NewDefaultTmpBackend(t)
-	defer betesting.Close(t, b)
+	b, bePath := betesting.NewDefaultTmpBackend(t)
 	s := mvcc.New(zap.NewExample(), b, &lease.FakeLessor{}, mvcc.StoreConfig{})
 	defer s.Close()
 
@@ -92,5 +95,29 @@ func TestWriteTxnPanic(t *testing.T) {
 		},
 	}
 
+	// compute DB file hash before applying the txn
+	dbHashBefore, err := computeFileHash(bePath)
+	require.NoErrorf(t, err, "failed to compute DB file hash before txn")
+
+	// we verify the following properties below:
+	// 1. server panics after a write txn aply fails (invariant: server should never try to move on from a failed write)
+	// 2. no writes from the txn are applied to the backend (invariant: failed write should have no side-effect on DB state besides panic)
 	assert.Panics(t, func() { a.Txn(ctx, txn) }, "Expected panic in Txn with writes")
+	dbHashAfter, err := computeFileHash(bePath)
+	require.NoErrorf(t, err, "failed to compute DB file hash after txn")
+	require.Equalf(t, dbHashBefore, dbHashAfter, "mismatch in DB hash before and after failed write txn")
+}
+
+func computeFileHash(filePath string) (string, error) {
+	file, err := os.Open(filePath)
+	if err != nil {
+		return "", err
+	}
+	defer file.Close()
+
+	h := sha256.New()
+	if _, err := io.Copy(h, file); err != nil {
+		return "", err
+	}
+	return string(h.Sum(nil)), nil
 }