fix(storage): align keystore cleanup semantics, error chains and facade tests (#2045)

Signed-off-by: AkramBitar <akram@il.ibm.com>

Author: AkramBitar

cmd/skicleanup/cobra/signers/runner.go

@@ -34,6 +34,8 @@ func Run(ctx context.Context, stores *Stores, batchSize int) error {
 	extractor := cleanup.NewSKIExtractor()
 	extractor.RegisterProvider(idemix.IdentityTypeString, idemix.NewSKIProvider())
 	extractor.RegisterProvider(idemixnym.IdentityTypeString, idemixnym.NewSKIProvider(stores.Identity))
+	// X.509 deliberately derives no SKIs, so orphaned X.509 signers report no keys to delete:
+	// an X.509 key belongs to the wallet, not to a single token. See cleanup.NoopSKIProvider.
 	extractor.RegisterProvider(x509.IdentityTypeString, cleanup.NewNoopSKIProvider())
 
 	var s stats

docs/services/storage/keystore_cleanup.md

@@ -53,14 +53,16 @@ The SKI (Subject Key Identifier) extraction system uses a pluggable provider arc
 3. **Built-in Providers**:
    - **IdemixSKIProvider**: Extracts SKI from Idemix NymPublicKey
    - **IdemixNymSKIProvider**: Extracts SKI from Idemix pseudonym identities
-   - **NoopSKIProvider**: Returns empty SKI list (used for X.509)
+   - **NoopSKIProvider**: Derives no SKIs, marking an identity type as deliberately excluded from
+     cleanup. Registered for X.509 — see [X.509 is intentionally out of scope](#x509-is-intentionally-out-of-scope)
    - **FallbackSKIProvider**: Computes SHA256 hash of identity bytes as SKI (default)
 
 **Provider Registration:**
 ```go
 extractor := NewSKIExtractor()
 extractor.RegisterProvider("idemix", idemix.NewSKIProvider())
 extractor.RegisterProvider("idemixnym", idemixnym.NewSKIProvider(identityStore))
+// X.509 keys belong to the wallet, not to individual tokens: deliberately never cleaned up
 extractor.RegisterProvider("x509", NewNoopSKIProvider())
 // Fallback provider is used for any unregistered types
 ```
@@ -72,6 +74,28 @@ extractor.RegisterProvider("x509", NewNoopSKIProvider())
 4. If not found, uses fallback provider (SHA256 hash)
 5. Returns list of SKI strings in hexadecimal format
 
+### X.509 is intentionally out of scope
+
+X.509 is registered with `NoopSKIProvider`, so **no keystore key is ever deleted for an
+X.509-owned token**. This is deliberate, not an unimplemented provider.
+
+An X.509 owner identity is a long-lived, non-anonymous certificate: `x509.KeyManager` reports
+`Anonymous() == false` and always serves the same identity descriptor. Its private key therefore
+belongs to the **wallet**, not to any individual token — the same key signs every token that
+wallet ever owns, and it remains in use long after those tokens are spent. A real X.509 SKI
+provider would make the cleanup sweep delete the wallet's own signing key as soon as the first of
+its tokens aged past the TTL, permanently breaking the wallet.
+
+The Idemix providers are the opposite case: their SKIs identify a one-shot pseudonym key created
+for a single recipient identity. That key is dead once its token is deleted, which is exactly what
+makes it safe to remove.
+
+**Consequence to be aware of:** a deleted X.509-owned token still gets a row in
+`token_ski_cleanups`, because the cleanup manager records "no key material to delete" the same way
+it records a completed deletion. For X.509 that row means *nothing to clean*, not *keys were
+removed*. Do not read the `token_ski_cleanups` table as evidence that X.509 key material was
+purged.
+
 ### Interfaces
 
 #### Storage Interface
@@ -264,7 +288,8 @@ cleanupManager := cleanup.NewServiceManager(
    - Get keystore for TMS
    - Derive SKIs from owner identity using appropriate provider
    - Delete each SKI from keystore
-   - Mark token as cleaned in database (even on partial success)
+   - Mark token as cleaned in database **only if every key was deleted** (or if the owner type has
+     no keys to delete at all)
 7. **Release Leadership**: Close leadership lock
 8. **Wait**: Sleep until next scan interval
 9. **Repeat**: Go to step 2
@@ -281,16 +306,26 @@ The cleanup service handles errors gracefully with specific retry behavior:
 
 ### Key Deletion Errors
 
-- **All keys fail to delete**: Token is NOT marked as cleaned; will retry on next sweep
-- **Some keys fail to delete**: Token IS marked as cleaned (partial success); logs warnings for failed keys
-- **No SKIs derived**: Token IS marked as cleaned to avoid infinite retries; logs warning
+Key deletion is **all-or-nothing** per token:
+
+- **Any key fails to delete**: Token is NOT marked as cleaned; the whole token is retried on the
+  next sweep. The returned error joins every per-key cause, so callers can inspect them with
+  `errors.Is`/`errors.As`
+- **All keys deleted**: Token IS marked as cleaned
+- **No SKIs derived**: Token IS marked as cleaned to avoid infinite retries; logs warning. This is
+  the normal path for X.509 — see
+  [X.509 is intentionally out of scope](#x509-is-intentionally-out-of-scope)
 
 ### Rationale
 
-This error handling strategy balances reliability with forward progress:
-- Complete failures trigger retries (transient errors may resolve)
-- Partial successes are recorded to avoid reprocessing successfully deleted keys
-- Empty SKI cases are marked complete to prevent infinite retry loops
+Marking a token cleaned while some of its key material is still in the keystore would turn a
+transient database error into a permanent, un-retriable key-retention hole: the token would never
+be selected again, so the surviving key would never be deleted. Retrying the whole token is safe
+and cheap because `Keystore.Delete` is idempotent — re-deleting the keys that already succeeded
+costs one no-op call each.
+
+Empty SKI cases are still marked complete, otherwise every sweep would rescan the same tokens
+forever.
 
 ### Other Errors
 
@@ -395,11 +430,12 @@ Key metrics to monitor:
 - **Error Rate**: Failed cleanup attempts (check logs for details)
 - **Leadership Changes**: Frequency of leader election (should be stable)
 - **Processing Time**: Duration of each cleanup sweep
-- **Partial Failures**: Tokens with some keys deleted but not all
+- **Retried Tokens**: Tokens that failed at least one key deletion and are still pending. A token
+  stuck here across many sweeps means a key deletion is failing persistently, not transiently
 
 **Log Levels:**
 - `INFO`: Successful cleanup operations, manager start/stop
-- `WARN`: Partial failures, key not found, leadership issues
+- `WARN`: Failed key deletions, key not found, leadership issues
 - `DEBUG`: Detailed sweep information, SKI derivation, leadership acquisition
 
 ## Security Considerations
@@ -408,7 +444,11 @@ Key metrics to monitor:
 - **Idempotency**: Safe to retry cleanup operations
 - **Audit Trail**: `token_ski_cleanups` table provides cleanup history with timestamps and instance tracking
 - **Key Isolation**: Only deletes keys for deleted tokens, never active tokens
-- **Partial Success Handling**: Prevents infinite retries while maintaining audit trail
+- **All-or-Nothing Marking**: A token is only recorded as cleaned once *all* of its keys are gone,
+  so a failed deletion can never be silently forgotten
+- **X.509 Exclusion**: X.509 key material is never deleted by design — a `token_ski_cleanups` row
+  for an X.509-owned token means "nothing to clean". See
+  [X.509 is intentionally out of scope](#x509-is-intentionally-out-of-scope)
 - **Instance Tracking**: `cleaned_by` field records which instance performed cleanup
 
 ## Comparison with Recovery Service

token/services/storage/endorserdb/store.go

@@ -112,7 +112,7 @@ func newStoreService(p dbdriver.EndorserStore) (*StoreService, error) {
 func (d *StoreService) ValidationRecords(ctx context.Context, params QueryValidationRecordsParams) (*ValidationRecordsIterator, error) {
 	it, err := d.db.QueryValidations(ctx, params)
 	if err != nil {
-		return nil, errors.Errorf("failed to query validation records: %s", err)
+		return nil, errors.Wrapf(err, "failed to query validation records")
 	}
 
 	return &ValidationRecordsIterator{it: it}, nil
@@ -152,9 +152,9 @@ func (d *StoreService) SetStatus(ctx context.Context, txID string, status dbdriv
 
 		return errors.Wrapf(err, "failed setting status [%s][%s]", txID, dbdriver.TxStatusMessage[status])
 	}
+	// No Rollback() here: once Commit() fails the driver transaction is already finalized, so
+	// rolling back is a guaranteed no-op. This matches AppendValidationRecord above and ttxdb.
 	if err := w.Commit(); err != nil {
-		w.Rollback()
-
 		return errors.Wrapf(err, "failed committing status [%s][%s]", txID, dbdriver.TxStatusMessage[status])
 	}
 

token/services/storage/endorserdb/store_internal_test.go

@@ -0,0 +1,57 @@
+/*
+Copyright IBM Corp. All Rights Reserved.
+
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package endorserdb
+
+import (
+	"context"
+	"testing"
+
+	"github.com/LFDT-Panurus/panurus/token/services/storage/db/common"
+	dbdriver "github.com/LFDT-Panurus/panurus/token/services/storage/db/driver"
+	"github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// errQueryFailed is a sentinel standing in for a driver-level cause a caller may want to match,
+// such as sql.ErrNoRows or a driver sentinel error.
+var errQueryFailed = errors.New("driver query failed")
+
+// failingEndorserStore is an EndorserStore whose QueryValidations always fails with
+// errQueryFailed.
+type failingEndorserStore struct{}
+
+func (failingEndorserStore) Close() error { return nil }
+
+func (failingEndorserStore) NewEndorserStoreTransaction() (dbdriver.EndorserStoreTransaction, error) {
+	return nil, errQueryFailed
+}
+
+func (failingEndorserStore) QueryValidations(context.Context, dbdriver.QueryValidationRecordsParams) (dbdriver.ValidationRecordsIterator, error) {
+	return nil, errQueryFailed
+}
+
+func (failingEndorserStore) GetStatus(context.Context, string) (dbdriver.TxStatus, string, error) {
+	return dbdriver.Unknown, "", errQueryFailed
+}
+
+// TestValidationRecordsPreservesErrorChain checks that a driver failure surfaced by
+// ValidationRecords stays matchable with errors.Is. Building the error by interpolating the cause
+// into a new message (errors.Errorf with %s) would break every caller that switches on the
+// underlying cause.
+func TestValidationRecordsPreservesErrorChain(t *testing.T) {
+	store := &StoreService{
+		StatusSupport: common.NewStatusSupport(),
+		db:            failingEndorserStore{},
+	}
+
+	it, err := store.ValidationRecords(t.Context(), QueryValidationRecordsParams{})
+	require.Error(t, err)
+	assert.Nil(t, it)
+	require.ErrorIs(t, err, errQueryFailed, "the driver cause must remain unwrappable")
+	assert.Contains(t, err.Error(), "failed to query validation records")
+}

token/services/storage/endorserdb/store_test.go

@@ -0,0 +1,119 @@
+/*
+Copyright IBM Corp. All Rights Reserved.
+
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package endorserdb_test
+
+import (
+	"testing"
+
+	"github.com/LFDT-Panurus/panurus/token"
+	driver2 "github.com/LFDT-Panurus/panurus/token/driver"
+	"github.com/LFDT-Panurus/panurus/token/sdk/tms"
+	config2 "github.com/LFDT-Panurus/panurus/token/services/config"
+	"github.com/LFDT-Panurus/panurus/token/services/storage/db/multiplexed"
+	"github.com/LFDT-Panurus/panurus/token/services/storage/db/sql/sqlite"
+	"github.com/LFDT-Panurus/panurus/token/services/storage/endorserdb"
+	"github.com/hyperledger-labs/fabric-smart-client/platform/view/services/config"
+	sqlite2 "github.com/hyperledger-labs/fabric-smart-client/platform/view/services/storage/driver/sql/sqlite"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	_ "modernc.org/sqlite"
+)
+
+// newStoreServiceManager builds a store service manager backed by the SQLite configuration in
+// ./testdata/sqlite.
+func newStoreServiceManager(t *testing.T) endorserdb.StoreServiceManager {
+	t.Helper()
+	// create a new config service by loading the config file
+	cp, err := config.NewProvider("./testdata/sqlite")
+	require.NoError(t, err)
+
+	return endorserdb.NewStoreServiceManager(
+		tms.NewConfigServiceWrapper(config2.NewService(cp)),
+		multiplexed.NewDriver(cp, sqlite.NewNamedDriver(cp, sqlite2.NewDbProvider())),
+	)
+}
+
+func TestDB(t *testing.T) {
+	manager := newStoreServiceManager(t)
+	_, err := manager.StoreServiceByTMSId(token.TMSID{Network: "pineapple", Namespace: "ns"})
+	require.NoError(t, err)
+	_, err = manager.StoreServiceByTMSId(token.TMSID{Network: "grapes", Namespace: "ns"})
+	require.NoError(t, err)
+}
+
+// TestValidationRecordsLifecycle exercises the facade end to end against SQLite:
+// AppendValidationRecord, GetStatus, SetStatus and ValidationRecords.
+func TestValidationRecordsLifecycle(t *testing.T) {
+	ctx := t.Context()
+	manager := newStoreServiceManager(t)
+	store, err := manager.StoreServiceByTMSId(token.TMSID{Network: "pineapple", Namespace: "ns"})
+	require.NoError(t, err)
+
+	meta := map[string][]byte{"key": []byte("value")}
+	require.NoError(t, store.AppendValidationRecord(ctx, "lifecycle-1", []byte("tr1"), meta, driver2.PPHash("pp")))
+	require.NoError(t, store.AppendValidationRecord(ctx, "lifecycle-2", []byte("tr2"), nil, driver2.PPHash("pp")))
+
+	// a freshly appended record is pending
+	status, message, err := store.GetStatus(ctx, "lifecycle-1")
+	require.NoError(t, err)
+	assert.Equal(t, endorserdb.Pending, status)
+	assert.Empty(t, message)
+
+	// the records are queryable, with metadata and token request round-tripped
+	records := readValidationRecords(t, store, endorserdb.QueryValidationRecordsParams{
+		Filter: func(record *endorserdb.ValidationRecord) bool {
+			return record.TxID == "lifecycle-1" || record.TxID == "lifecycle-2"
+		},
+	})
+	require.Len(t, records, 2)
+	assert.Equal(t, "lifecycle-1", records[0].TxID)
+	assert.Equal(t, []byte("tr1"), records[0].TokenRequest)
+	assert.Equal(t, meta, records[0].Metadata)
+	assert.Equal(t, "lifecycle-2", records[1].TxID)
+	assert.Equal(t, []byte("tr2"), records[1].TokenRequest)
+
+	// SetStatus is reflected by GetStatus...
+	require.NoError(t, store.SetStatus(ctx, "lifecycle-1", endorserdb.Confirmed, "all good"))
+	status, message, err = store.GetStatus(ctx, "lifecycle-1")
+	require.NoError(t, err)
+	assert.Equal(t, endorserdb.Confirmed, status)
+	assert.Equal(t, "all good", message)
+
+	// ...and by the Statuses filter of ValidationRecords
+	confirmed := readValidationRecords(t, store, endorserdb.QueryValidationRecordsParams{
+		Statuses: []endorserdb.TxStatus{endorserdb.Confirmed},
+		Filter: func(record *endorserdb.ValidationRecord) bool {
+			return record.TxID == "lifecycle-1" || record.TxID == "lifecycle-2"
+		},
+	})
+	require.Len(t, confirmed, 1)
+	assert.Equal(t, "lifecycle-1", confirmed[0].TxID)
+
+	// the sibling record is untouched
+	status, _, err = store.GetStatus(ctx, "lifecycle-2")
+	require.NoError(t, err)
+	assert.Equal(t, endorserdb.Pending, status)
+}
+
+func readValidationRecords(t *testing.T, store *endorserdb.StoreService, params endorserdb.QueryValidationRecordsParams) []*endorserdb.ValidationRecord {
+	t.Helper()
+	it, err := store.ValidationRecords(t.Context(), params)
+	require.NoError(t, err)
+	defer it.Close()
+
+	var records []*endorserdb.ValidationRecord
+	for {
+		next, err := it.Next()
+		require.NoError(t, err)
+		if next == nil {
+			break
+		}
+		records = append(records, next)
+	}
+
+	return records
+}

token/services/storage/endorserdb/testdata/sqlite/core.yaml

@@ -0,0 +1,24 @@
+fsc:
+  persistences:
+    token_persistence:
+      type: sqlite
+      opts:
+        dataSource: file:tmp?_pragma=journal_mode(WAL)&_pragma=busy_timeout(20000)&mode=memory&cache=shared
+        tablePrefix: tsdk
+        maxOpenConns: 10
+
+token:
+  enabled: true
+  tms:
+    pineapple:
+      network: pineapple
+      channel:
+      namespace: ns
+      endorserdb:
+        persistence: token_persistence
+    grapes:
+      network: grapes
+      channel:
+      namespace: ns
+      endorserdb:
+        persistence: token_persistence

token/services/storage/services/cleanup/manager.go

@@ -47,7 +47,7 @@ type Leadership interface {
 	Close() error
 }
 
-//go:generate counterfeiter -o mock/identity_provider.go -fake-name IdentityProvider . IdentityProvider
+//go:generate counterfeiter -o mock/identity_provider.go -fake-name IdentityProvider . SKIProvider
 
 // SKIProvider provides methods to derive SKIs from identities
 type SKIProvider interface {
@@ -326,7 +326,9 @@ func (m *Manager) cleanupToken(ctx context.Context, token DeletedToken) error {
 
 	if len(skis) == 0 {
 		m.logger.Warnf("no SKIs derived for token [%s:%d], skipping", token.TxID, token.Index)
-		// Still mark as cleaned to avoid retrying
+		// No SKIs means this owner type has no per-token key material to remove (for example
+		// X.509, see NoopSKIProvider). Mark the token as cleaned to avoid rescanning it on
+		// every sweep: "cleaned" here records "nothing to delete", not "keys were deleted".
 		if err := m.storage.MarkTokenCleaned(ctx, token.TxID, token.Index, m.config.InstanceID); err != nil {
 			return errors.Wrapf(err, "failed to mark token [%s:%d] as cleaned", token.TxID, token.Index)
 		}
@@ -347,12 +349,20 @@ func (m *Manager) cleanupToken(ctx context.Context, token DeletedToken) error {
 		}
 	}
 
-	// If all deletions failed, return error without marking as cleaned
+	// All-or-nothing: if *any* deletion failed, return an error without marking the token as
+	// cleaned, so the next sweep retries it. Marking a token cleaned while some of its key
+	// material is still in the keystore would turn a transient error into a permanent,
+	// un-retriable key-retention hole. Re-deleting the keys that did succeed is harmless,
+	// because Keystore.Delete is idempotent.
 	if len(deleteErrors) > 0 {
-		return errors.Errorf("failed to delete keys for token [%s:%d]: %v", token.TxID, token.Index, deleteErrors)
+		return errors.Wrapf(
+			errors.Join(deleteErrors...),
+			"failed to delete %d of %d key(s) for token [%s:%d]",
+			len(deleteErrors), len(skis), token.TxID, token.Index,
+		)
 	}
 
-	// Mark token as cleaned (even if some keys failed to delete)
+	// Every key was deleted: record the token as cleaned so it is not reprocessed.
 	if err := m.storage.MarkTokenCleaned(ctx, token.TxID, token.Index, m.config.InstanceID); err != nil {
 		return errors.Wrapf(err, "failed to mark token [%s:%d] as cleaned", token.TxID, token.Index)
 	}