fix(spanner): guard rollback when aborted commit cleared session handle (#14218)

rahul2393 · web-flow · commit 63151055c1d2 · 2026-03-20T14:09:33.000+05:30
Fixes: #14025 Fix a panic in `ReadWriteTransaction.rollback` when a statement-based transaction commit is aborted, commit cleanup clears `t.sh`, and a later rollback is triggered after retry reset fails. ## Root cause `ReadWriteStmtBasedTransaction.CommitWithReturnResp` recycles the session handle and sets `t.sh = nil` even when commit returns `ABORTED`. If a caller then tries to retry with a canceled context, `ResetForRetry` fails before a new transaction is created. Cleanup paths can still call `Rollback` on the original transaction. `ReadWriteTransaction.rollback` dereferenced `t.sh` without checking whether the handle itself was nil, which caused a panic. ## Change - Snapshot `t.sh` inside `ReadWriteTransaction.rollback` - Return early if the session handle is nil - Use the local snapshot for `getID`, `getClient`, and `getMetadata` This keeps rollback a no-op when the session handle has already been cleaned up, which matches the intended behavior for an already-aborted transaction.
diff --git a/spanner/transaction.go b/spanner/transaction.go
@@ -1932,14 +1932,18 @@ func (t *ReadWriteTransaction) rollback(ctx context.Context) {
 		t.mu.Unlock()
 		return
 	}
+	sh := t.sh
 	t.mu.Unlock()
 	// In case that sessionHandle was destroyed but transaction body fails to
 	// report it.
-	sid, client := t.sh.getID(), t.sh.getClient()
+	if sh == nil {
+		return
+	}
+	sid, client := sh.getID(), sh.getClient()
 	if sid == "" || client == nil {
 		return
 	}
-	_ = client.Rollback(contextWithOutgoingMetadata(ctx, t.sh.getMetadata(), t.disableRouteToLeader), &sppb.RollbackRequest{
+	_ = client.Rollback(contextWithOutgoingMetadata(ctx, sh.getMetadata(), t.disableRouteToLeader), &sppb.RollbackRequest{
 		Session:       sid,
 		TransactionId: t.tx,
 	})
diff --git a/spanner/transaction_test.go b/spanner/transaction_test.go
@@ -728,6 +728,57 @@ func TestReadWriteStmtBasedTransaction_CommitAbortedErrorReturned(t *testing.T)
 	}
 }
 
+// When a commit is aborted and the retry fails because the context is
+// cancelled, calling Rollback on the transaction should not panic even
+// though the session handle has been set to nil.
+func TestReadWriteStmtBasedTransaction_RollbackAfterAbortedCommitWithNilSessionHandle(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	server, client, teardown := setupMockedTestServer(t)
+	defer teardown()
+	// Make the commit return an Aborted error.
+	server.TestSpanner.PutExecutionTime(MethodCommitTransaction,
+		SimulatedExecutionTime{
+			Errors: []error{status.Errorf(codes.Aborted, "Transaction aborted")},
+		})
+
+	txn, err := NewReadWriteStmtBasedTransaction(ctx, client)
+	if err != nil {
+		t.Fatalf("got an error: %v", err)
+	}
+	// Commit should return Aborted. This also sets txn.sh = nil internally.
+	_, err = txn.Commit(ctx)
+	if status.Code(err) != codes.Aborted {
+		t.Fatalf("got an incorrect error: %v", err)
+	}
+	if txn.sh != nil {
+		t.Fatal("expected aborted commit cleanup to clear the session handle")
+	}
+
+	// Try to reset for retry with a cancelled context, simulating the
+	// scenario where the caller's context has been cancelled (e.g. by
+	// errgroup). This will fail because session acquisition needs a
+	// valid context.
+	cancelledCtx, cancel := context.WithCancel(ctx)
+	cancel()
+	_, err = txn.ResetForRetry(cancelledCtx)
+	if g, w := ErrCode(err), codes.Canceled; g != w {
+		t.Fatalf("ResetForRetry error code mismatch\n Got: %v\nWant: %v", g, w)
+	}
+
+	// Rollback should not panic even though txn.sh is nil.
+	var recovered any
+	func() {
+		defer func() {
+			recovered = recover()
+		}()
+		txn.Rollback(context.Background())
+	}()
+	if recovered != nil {
+		t.Fatalf("Rollback panicked after failed retry reset: %v", recovered)
+	}
+}
+
 // When a non-aborted error happens during a commit, it kicks off a rollback.
 func TestReadWriteStmtBasedTransaction_CommitNonAbortedErrorReturned(t *testing.T) {
 	t.Parallel()
