Review comments

Signed-off-by: AKRAM@il.ibm.com <akram@akramb.vpc.cloud9.ibm.com>

Author: AKRAM@il.ibm.com

integration/token/fungible/views/auditor.go

@@ -57,11 +57,9 @@ func (a *AuditView) Call(context view.Context) (interface{}, error) {
 	// extract inputs and outputs
 	logger.Debugf("AuditView: audit [%s]", tx.ID())
 	inputs, outputs, err := auditor.Audit(context.Context(), tx)
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed retrieving inputs and outputs")
-	}
-	defer auditor.Release(context.Context(), tx)
+	assert.NoError(err, "failed retrieving inputs and outputs")
 	logger.Debugf("AuditView: audit done [%s]", tx.ID())
+	defer auditor.Release(context.Context(), tx)
 
 	logger.Debugf("AuditView: [%s] get query executor... ", tx.ID())
 

integration/token/interop/views/auditor.go

@@ -11,7 +11,6 @@ import (
 	"math/big"
 	"time"
 
-	"github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors"
 	"github.com/hyperledger-labs/fabric-smart-client/platform/common/utils/assert"
 	"github.com/hyperledger-labs/fabric-smart-client/platform/view/view"
 	"github.com/hyperledger-labs/fabric-token-sdk/token"
@@ -41,9 +40,7 @@ func (a *AuditView) Call(context view.Context) (interface{}, error) {
 
 	// extract inputs and outputs
 	inputs, outputs, err := auditor.Audit(context.Context(), tx)
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed retrieving inputs and outputs")
-	}
+	assert.NoError(err, "failed retrieving inputs and outputs")
 	defer auditor.Release(context.Context(), tx)
 
 	// For example, all payments of an amount less than or equal to payment limit is valid

token/services/auditor/auditor_test.go

@@ -719,3 +719,240 @@ func TestManager_GetByTMSID(t *testing.T) {
 		auditor.Get(sp, w)
 	})
 }
+
+// ---------------------------------------------------------------------------
+// Service.Audit Lock Management Tests
+// ---------------------------------------------------------------------------
+
+// TestService_Audit_LocksReleasedOnAuditRecordError verifies that when Audit() fails
+// before lock acquisition (during AuditRecord()), no locks are held and Release() is safe.
+// Audit() acquires locks ONLY after successful AuditRecord(), so early failures don't leak locks.
+func TestService_Audit_LocksReleasedOnAuditRecordError(t *testing.T) {
+	// Create a TMS that will fail on AuditRecord by returning nil public parameters
+	mockTMS := &drivermock.TokenManagerService{}
+	mockPPM := &drivermock.PublicParamsManager{}
+	mockPPM.PublicParametersReturns(nil) // This will cause AuditRecord to fail
+	mockTMS.PublicParamsManagerReturns(mockPPM)
+	mockTMS.ValidatorReturns(&drivermock.Validator{}, nil)
+	mockTMS.TokensServiceReturns(&drivermock.TokensService{})
+	mockTMS.WalletServiceReturns(&drivermock.WalletService{})
+
+	mockVP := &tokenmock.VaultProvider{}
+	mockV := &drivermock.Vault{}
+	mockV.QueryEngineReturns(&drivermock.QueryEngine{})
+	mockVP.VaultReturns(mockV, nil)
+
+	badTMS, err := token.NewManagementService(
+		token.TMSID{}, mockTMS, logging.MustGetLogger("test"), mockVP, nil, nil,
+	)
+	require.NoError(t, err)
+
+	storeService := newTestStoreService(t, newFakeStore())
+	svc := newTestService(storeService, nil)
+
+	tx := &auditmock.Transaction{}
+	tx.IDReturns("tx-audit-record-err")
+	tx.RequestReturns(token.NewRequest(badTMS, token.RequestAnchor("tx-audit-record-err")))
+
+	// Audit should fail
+	_, _, err = svc.Audit(context.Background(), tx)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "failed getting transaction audit record")
+
+	// Release should be safe to call even though Audit failed
+	assert.NotPanics(t, func() {
+		svc.Release(context.Background(), tx)
+	})
+
+	// Verify no locks are held by trying to acquire the same anchor
+	ctx := context.Background()
+	err = storeService.AcquireLocks(ctx, "tx-audit-record-err")
+	require.NoError(t, err, "should be able to acquire locks since Audit failed")
+	storeService.ReleaseLocks(ctx, "tx-audit-record-err")
+}
+
+// TestService_Audit_LocksAcquiredOnSuccess verifies successful Audit() acquires locks,
+// Release() frees them, and Release() is idempotent (safe to call multiple times).
+func TestService_Audit_LocksAcquiredOnSuccess(t *testing.T) {
+	storeService := newTestStoreService(t, newFakeStore())
+	svc := newTestService(storeService, nil)
+
+	tx := &auditmock.Transaction{}
+	tx.IDReturns("tx-audit-success")
+	tx.RequestReturns(token.NewRequest(newTestManagementService(t), token.RequestAnchor("tx-audit-success")))
+
+	ctx := context.Background()
+
+	// Audit should succeed
+	inputs, outputs, err := svc.Audit(ctx, tx)
+	require.NoError(t, err)
+	assert.NotNil(t, inputs)
+	assert.NotNil(t, outputs)
+
+	// Verify Release is safe to call
+	assert.NotPanics(t, func() {
+		svc.Release(ctx, tx)
+	})
+
+	// Verify Release is idempotent
+	assert.NotPanics(t, func() {
+		svc.Release(ctx, tx)
+	})
+}
+
+// TestService_Audit_ContextCancellationBeforeLockAcquisition verifies context cancellation
+// doesn't leak locks. Semaphore auto-rolls back partially acquired locks (PR #1616).
+// Release() is always safe regardless of Audit() outcome.
+func TestService_Audit_ContextCancellationBeforeLockAcquisition(t *testing.T) {
+	storeService := newTestStoreService(t, newFakeStore())
+	svc := newTestService(storeService, nil)
+
+	tx := &auditmock.Transaction{}
+	tx.IDReturns("tx-ctx-cancel")
+	tx.RequestReturns(token.NewRequest(newTestManagementService(t), token.RequestAnchor("tx-ctx-cancel")))
+
+	// Use a cancelled context
+	cancelledCtx, cancel := context.WithCancel(context.Background())
+	cancel()
+
+	// Audit may fail due to context cancellation (depending on timing)
+	// or succeed if AuditRecord completes before cancellation check
+	_, _, _ = svc.Audit(cancelledCtx, tx)
+	// We don't assert error here as it depends on timing
+
+	// Release should always be safe to call
+	assert.NotPanics(t, func() {
+		svc.Release(context.Background(), tx)
+	})
+
+	// Verify we can acquire locks after (no locks were leaked)
+	ctx := context.Background()
+	err := storeService.AcquireLocks(ctx, "tx-ctx-cancel")
+	require.NoError(t, err, "should be able to acquire locks")
+	storeService.ReleaseLocks(ctx, "tx-ctx-cancel")
+}
+
+// TestService_Audit_MultipleAuditsSequential verifies sequential audits work correctly:
+// first Audit() acquires locks, Release() frees them, second Audit() succeeds.
+func TestService_Audit_MultipleAuditsSequential(t *testing.T) {
+	storeService := newTestStoreService(t, newFakeStore())
+	svc := newTestService(storeService, nil)
+
+	ctx := context.Background()
+
+	// First audit
+	tx1 := &auditmock.Transaction{}
+	tx1.IDReturns("tx-audit-1")
+	tx1.RequestReturns(token.NewRequest(newTestManagementService(t), token.RequestAnchor("tx-audit-1")))
+
+	inputs1, outputs1, err := svc.Audit(ctx, tx1)
+	require.NoError(t, err)
+	assert.NotNil(t, inputs1)
+	assert.NotNil(t, outputs1)
+
+	// Release first audit's locks
+	svc.Release(ctx, tx1)
+
+	// Second audit should succeed
+	tx2 := &auditmock.Transaction{}
+	tx2.IDReturns("tx-audit-2")
+	tx2.RequestReturns(token.NewRequest(newTestManagementService(t), token.RequestAnchor("tx-audit-2")))
+
+	inputs2, outputs2, err := svc.Audit(ctx, tx2)
+	require.NoError(t, err)
+	assert.NotNil(t, inputs2)
+	assert.NotNil(t, outputs2)
+
+	// Clean up
+	svc.Release(ctx, tx2)
+}
+
+// TestService_Audit_ReleaseIdempotency verifies Release() is idempotent - can be called
+// multiple times safely without panics (handles error paths, defer, retry logic).
+func TestService_Audit_ReleaseIdempotency(t *testing.T) {
+	storeService := newTestStoreService(t, newFakeStore())
+	svc := newTestService(storeService, nil)
+
+	tx := &auditmock.Transaction{}
+	tx.IDReturns("tx-release-idempotent")
+	tx.RequestReturns(token.NewRequest(newTestManagementService(t), token.RequestAnchor("tx-release-idempotent")))
+
+	// Audit to acquire locks
+	_, _, err := svc.Audit(context.Background(), tx)
+	require.NoError(t, err)
+
+	ctx := context.Background()
+
+	// First release should work
+	assert.NotPanics(t, func() {
+		svc.Release(ctx, tx)
+	})
+
+	// Second release should also be safe (no-op)
+	assert.NotPanics(t, func() {
+		svc.Release(ctx, tx)
+	})
+
+	// Third release should still be safe
+	assert.NotPanics(t, func() {
+		svc.Release(ctx, tx)
+	})
+}
+
+// TestService_Audit_ReleaseWithoutAudit verifies Release() is safe to call without
+// prior Audit() (handles defer in error paths where Audit() never ran or failed early).
+func TestService_Audit_ReleaseWithoutAudit(t *testing.T) {
+	storeService := newTestStoreService(t, newFakeStore())
+	svc := newTestService(storeService, nil)
+
+	tx := &auditmock.Transaction{}
+	tx.IDReturns("tx-no-audit")
+	tx.RequestReturns(token.NewRequest(newTestManagementService(t), token.RequestAnchor("tx-no-audit")))
+
+	// Release without Audit should be safe
+	assert.NotPanics(t, func() {
+		svc.Release(context.Background(), tx)
+	})
+}
+
+// TestService_Audit_PanicRecoveryReleasesLocks verifies defer Release() executes even
+// when code panics, preventing lock leaks. Demonstrates correct pattern:
+//
+//	defer auditor.Release(ctx, tx)  // MUST be after error check
+func TestService_Audit_PanicRecoveryReleasesLocks(t *testing.T) {
+	storeService := newTestStoreService(t, newFakeStore())
+	svc := newTestService(storeService, nil)
+
+	tx := &auditmock.Transaction{}
+	tx.IDReturns("tx-panic-recovery")
+	tx.RequestReturns(token.NewRequest(newTestManagementService(t), token.RequestAnchor("tx-panic-recovery")))
+
+	ctx := context.Background()
+
+	// Simulate code that panics after Audit but has defer Release
+	func() {
+		defer func() {
+			if r := recover(); r != nil {
+				// Panic recovered as expected
+				assert.Equal(t, "simulated panic", r)
+			}
+		}()
+
+		// Audit succeeds and acquires locks
+		inputs, outputs, err := svc.Audit(ctx, tx)
+		require.NoError(t, err)
+		assert.NotNil(t, inputs)
+		assert.NotNil(t, outputs)
+
+		// Defer Release - this should execute even if panic occurs
+		defer svc.Release(ctx, tx)
+
+		// Simulate panic in subsequent processing
+		panic("simulated panic")
+	}()
+
+	// Verify locks were released by attempting to acquire them
+	err := storeService.AcquireLocks(ctx, "tx-panic-recovery")
+	require.NoError(t, err, "locks should have been released despite panic")
+	storeService.ReleaseLocks(ctx, "tx-panic-recovery")
+}