fix: empty transaction metadata should short circuit the conclude call

Signed-off-by: Harshit Gangal <harshit@planetscale.com>
vitessio · Sep 16, 2024 · ae06dd1 · ae06dd1
1 parent 5e1d4a9
commit ae06dd1
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 36 deletions.
diff --git a/go/vt/vtctl/grpcvtctldserver/server.go b/go/vt/vtctl/grpcvtctldserver/server.go
@@ -2462,7 +2462,8 @@ func (s *VtctldServer) ConcludeTransaction(ctx context.Context, req *vtctldatapb
 	if len(participants) == 0 {
 		// Read the transaction metadata if participating resource manager list is not provided in the request.
 		transaction, err := s.tmc.ReadTransaction(ctx, primary.Tablet, req.Dtid)
-		if err != nil {
+		if transaction == nil || err != nil {
+			// no transaction record for the given ID. It is already concluded or does not exist.
 			return nil, err
 		}
 		participants = transaction.Participants

diff --git a/go/vt/vtctl/grpcvtctldserver/server_test.go b/go/vt/vtctl/grpcvtctldserver/server_test.go
@@ -5257,34 +5257,47 @@ func TestConcludeTransaction(t *testing.T) {
 		dtid        string
 		expErr      string
 		participant []*querypb.Target
-	}{
-		{
-			name:   "invalid dtid",
-			tmc:    &testutil.TabletManagerClient{},
-			dtid:   "dtid01",
-			expErr: "invalid parts in dtid: dtid01",
-		}, {
-			name:   "invalid transaction id",
-			tmc:    &testutil.TabletManagerClient{},
-			dtid:   "ks:80-:013c",
-			expErr: "invalid transaction id in dtid: ks:80-:013c",
-		}, {
-			name: "only dtid",
-			tmc:  &testutil.TabletManagerClient{},
-			dtid: "testkeyspace:80-:1234",
-		}, {
-			name:        "with participant",
-			tmc:         &testutil.TabletManagerClient{},
-			dtid:        "testkeyspace:80-:1234",
-			participant: []*querypb.Target{{Keyspace: ks, Shard: "-80"}},
-		}, {
-			name:        "call error",
-			tmc:         &testutil.TabletManagerClient{CallError: true},
-			dtid:        "testkeyspace:80-:1234",
-			participant: []*querypb.Target{{Keyspace: ks, Shard: "-80"}},
-			expErr:      "blocked call for ConcludeTransaction on fake TabletManagerClient",
+	}{{
+		name:   "invalid dtid",
+		tmc:    &testutil.TabletManagerClient{},
+		dtid:   "dtid01",
+		expErr: "invalid parts in dtid: dtid01",
+	}, {
+		name:   "invalid transaction id",
+		tmc:    &testutil.TabletManagerClient{},
+		dtid:   "ks:80-:013c",
+		expErr: "invalid transaction id in dtid: ks:80-:013c",
+	}, {
+		name: "only dtid",
+		tmc: &testutil.TabletManagerClient{
+			ReadTransactionResult: map[string]*querypb.TransactionMetadata{
+				"80-": {Dtid: "bb"},
+			},
 		},
-	}
+		dtid: "testkeyspace:80-:1234",
+	}, {
+		name: "only dtid - empty metadata",
+		tmc: &testutil.TabletManagerClient{
+			ReadTransactionResult: map[string]*querypb.TransactionMetadata{},
+		},
+		dtid: "testkeyspace:80-:1234",
+	}, {
+		name:   "only dtid - fail",
+		tmc:    &testutil.TabletManagerClient{},
+		dtid:   "testkeyspace:80-:1234",
+		expErr: "no ReadTransaction result on fake TabletManagerClient",
+	}, {
+		name:        "with participant",
+		tmc:         &testutil.TabletManagerClient{},
+		dtid:        "testkeyspace:80-:1234",
+		participant: []*querypb.Target{{Keyspace: ks, Shard: "-80"}},
+	}, {
+		name:        "call error",
+		tmc:         &testutil.TabletManagerClient{CallError: true},
+		dtid:        "testkeyspace:80-:1234",
+		participant: []*querypb.Target{{Keyspace: ks, Shard: "-80"}},
+		expErr:      "blocked call for ConcludeTransaction on fake TabletManagerClient",
+	}}
 
 	tablets := []*topodatapb.Tablet{{
 		Alias:    &topodatapb.TabletAlias{Cell: "zone1", Uid: 100},

diff --git a/go/vt/vtctl/grpcvtctldserver/testutil/test_tmclient.go b/go/vt/vtctl/grpcvtctldserver/testutil/test_tmclient.go
@@ -260,6 +260,7 @@ type TabletManagerClient struct {
 		Error    error
 	}
 	GetUnresolvedTransactionsResults map[string][]*querypb.TransactionMetadata
+	ReadTransactionResult            map[string]*querypb.TransactionMetadata
 	// keyed by tablet alias.
 	InitPrimaryDelays map[string]time.Duration
 	// keyed by tablet alias. injects a sleep to the end of the function
@@ -663,20 +664,24 @@ func (fake *TabletManagerClient) GetUnresolvedTransactions(ctx context.Context,
 	return fake.GetUnresolvedTransactionsResults[tablet.Shard], nil
 }
 
-// ConcludeTransaction is part of the tmclient.TabletManagerClient interface.
-func (fake *TabletManagerClient) ConcludeTransaction(ctx context.Context, tablet *topodatapb.Tablet, dtid string, mm bool) error {
+// ReadTransaction is part of the tmclient.TabletManagerClient interface.
+func (fake *TabletManagerClient) ReadTransaction(ctx context.Context, tablet *topodatapb.Tablet, dtid string) (*querypb.TransactionMetadata, error) {
 	if fake.CallError {
-		return fmt.Errorf("%w: blocked call for ConcludeTransaction on fake TabletManagerClient", assert.AnError)
+		return nil, fmt.Errorf("%w: blocked call for ReadTransaction on fake TabletManagerClient", assert.AnError)
 	}
-	return nil
+	if fake.ReadTransactionResult == nil {
+		return nil, fmt.Errorf("%w: no ReadTransaction result on fake TabletManagerClient", assert.AnError)
+	}
+
+	return fake.ReadTransactionResult[tablet.Shard], nil
 }
 
-// ReadTransaction is part of the tmclient.TabletManagerClient interface.
-func (fake *TabletManagerClient) ReadTransaction(ctx context.Context, tablet *topodatapb.Tablet, dtid string) (*querypb.TransactionMetadata, error) {
+// ConcludeTransaction is part of the tmclient.TabletManagerClient interface.
+func (fake *TabletManagerClient) ConcludeTransaction(ctx context.Context, tablet *topodatapb.Tablet, dtid string, mm bool) error {
 	if fake.CallError {
-		return nil, fmt.Errorf("%w: blocked call for ReadTransaction on fake TabletManagerClient", assert.AnError)
+		return fmt.Errorf("%w: blocked call for ConcludeTransaction on fake TabletManagerClient", assert.AnError)
 	}
-	return nil, nil
+	return nil
 }
 
 // FullStatus is part of the tmclient.TabletManagerClient interface.