fix(ttx): close endorsement sessions on all return paths (LFDT-Panurus#1893)

Author: AkramBitar

token/services/ttx/cleanupsessions_test.go

@@ -0,0 +1,77 @@
+/*
+Copyright IBM Corp. All Rights Reserved.
+
+SPDX-License-Identifier: Apache-2.0
+*/
+
+// This white-box (package ttx) file covers cleanupSessions, which needs access to
+// the unexported sessions field. It is kept separate from the black-box
+// collectendorsements_test.go so the latter can keep importing dep/mock, which
+// cannot be imported from package ttx without creating an import cycle.
+package ttx
+
+import (
+	"context"
+	"testing"
+
+	"github.com/hyperledger-labs/fabric-smart-client/platform/view/view"
+	"github.com/stretchr/testify/assert"
+)
+
+// countingSession is a minimal view.Session fake that records how many times
+// Close was called.
+type countingSession struct {
+	closes int
+}
+
+func (s *countingSession) Info() view.SessionInfo                             { return view.SessionInfo{} }
+func (s *countingSession) Send([]byte) error                                  { return nil }
+func (s *countingSession) SendWithContext(context.Context, []byte) error      { return nil }
+func (s *countingSession) SendError([]byte) error                             { return nil }
+func (s *countingSession) SendErrorWithContext(context.Context, []byte) error { return nil }
+func (s *countingSession) Receive() <-chan *view.Message                      { return nil }
+func (s *countingSession) Close()                                             { s.closes++ }
+
+// TestCleanupSessions_ClosesAllAndEmptiesMap verifies that cleanupSessions closes
+// every tracked session and clears the session map, so no session is leaked on any
+// return path of Call.
+func TestCleanupSessions_ClosesAllAndEmptiesMap(t *testing.T) {
+	s1, s2 := &countingSession{}, &countingSession{}
+	c := &CollectEndorsementsView{
+		sessions: map[string]view.Session{"auditor": s1, "party": s2},
+	}
+
+	c.cleanupSessions(t.Context())
+
+	assert.Equal(t, 1, s1.closes, "auditor session should be closed exactly once")
+	assert.Equal(t, 1, s2.closes, "party session should be closed exactly once")
+	assert.Empty(t, c.sessions, "session map should be emptied after cleanup")
+}
+
+// TestCleanupSessions_Idempotent verifies that a second cleanupSessions call is a
+// no-op and does not close any session twice. This matters because the deferred
+// cleanup may run after sessions were already released earlier in the flow.
+func TestCleanupSessions_Idempotent(t *testing.T) {
+	s := &countingSession{}
+	c := &CollectEndorsementsView{
+		sessions: map[string]view.Session{"auditor": s},
+	}
+
+	c.cleanupSessions(t.Context())
+	c.cleanupSessions(t.Context())
+
+	assert.Equal(t, 1, s.closes, "session must not be closed more than once across repeated cleanup calls")
+}
+
+// TestCleanupSessions_NilAndEmptySafe verifies cleanupSessions tolerates an empty
+// map and nil session entries without panicking.
+func TestCleanupSessions_NilAndEmptySafe(t *testing.T) {
+	// Empty map.
+	empty := &CollectEndorsementsView{sessions: map[string]view.Session{}}
+	assert.NotPanics(t, func() { empty.cleanupSessions(t.Context()) })
+
+	// Nil session entry.
+	withNil := &CollectEndorsementsView{sessions: map[string]view.Session{"nil": nil}}
+	assert.NotPanics(t, func() { withNil.cleanupSessions(t.Context()) })
+	assert.Empty(t, withNil.sessions, "nil entries should also be removed")
+}

token/services/ttx/collectendorsements.go

@@ -101,6 +101,10 @@ func (c *CollectEndorsementsView) Call(context view.Context) (any, error) {
 	// Ensure Done() is called on all external wallets regardless of errors
 	defer c.CleanupExternalWallets(context, externalWallets)
 
+	// Close all endorsement sessions on every return path. Leaking the auditor
+	// session on an error path keeps its audit-DB enrollment-ID lock held.
+	defer c.cleanupSessions(context.Context())
+
 	// 1. First collect signatures on the token request
 	issueSigmas, err := c.requestSignaturesOnIssues(context, externalWallets)
 	if err != nil {
@@ -142,14 +146,7 @@ func (c *CollectEndorsementsView) Call(context view.Context) (any, error) {
 		return nil, errors.WithMessagef(err, "failed distributing tx")
 	}
 
-	// Cleanup audit
-	logger.DebugfContext(context.Context(), "Cleanup audit")
-	if err := c.cleanupAudit(context); err != nil {
-		logger.ErrorfContext(context.Context(), "failed cleaning up audit: %s", err)
-
-		return nil, errors.WithMessagef(err, "failed cleaning up audit")
-	}
-
+	// Sessions are closed by the deferred cleanupSessions call.
 	logger.DebugfContext(context.Context(), "CollectEndorsementsView done.")
 
 	labels := []string{
@@ -451,18 +448,18 @@ func (c *CollectEndorsementsView) requestAudit(context view.Context) ([]view.Ide
 	return nil, nil
 }
 
-// cleanupAudit closes the auditor session if one was opened during the audit process.
-// This should be called after the transaction has been fully endorsed and distributed.
-func (c *CollectEndorsementsView) cleanupAudit(context view.Context) error {
-	if !c.tx.Opts.Auditor.IsNone() {
-		session, err := c.getSession(context, c.tx.Opts.Auditor)
-		if err != nil {
-			return errors.Wrap(err, "failed getting auditor's session")
+// cleanupSessions closes and removes every session opened while collecting
+// endorsements (e.g. the auditor session). Deleting entries as they are closed
+// makes it idempotent, so the deferred call is safe to run on any return path.
+func (c *CollectEndorsementsView) cleanupSessions(ctx context.Context) {
+	for key, session := range c.sessions {
+		delete(c.sessions, key)
+		if session == nil {
+			continue
 		}
+		logger.DebugfContext(ctx, "closing endorsement session [%s]", key)
 		session.Close()
 	}
-
-	return nil
 }
 
 // distributeTxToParties distributes the endorsed transaction to all parties in the distribution list.