fix(tokens): always report a failed token delete

DBTransaction.DeleteToken only returned a Delete error when the preceding
GetToken lookup had found the token. GetToken returns (nil, owners, nil)
whenever the token is absent from the local store, which is the ordinary
outcome for inputs that are not mine and, under graph hiding, for spent
identifiers that were never stored locally. On that common path the code
discarded a genuine Delete failure and returned nil, so a transient
storage error let AppendValid report success and the surrounding
transaction be recorded as processed while the spend was never applied.

Delete is documented and implemented as idempotent, so marking an unknown
token as spent is not an error and any error it returns is a real storage
failure. Propagate it unconditionally, and keep the token-absent check
only where it belongs: skipping the delete-token notification.

Fixes #2182

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

Author: AkramBitar

docs/services/tokens.md

@@ -12,6 +12,15 @@ The internal [Service](../../token/services/tokens/tokens.go) is responsible for
 *   **Lifecycle Monitoring**: Notifying local listeners (via `events.Publisher`) when tokens are added or removed.
 *   **Consistency**: Identifying and removing stale unspent tokens by cross-referencing local storage with the ledger via the [Network Service](../../token/services/network/network.go) (see `PruneInvalidUnspentTokens`).
 
+### Marking Spent Tokens
+
+`DBTransaction.DeleteToken` marks a spent token as deleted. Deletion is *idempotent*: a
+token that is not present in the local store — the ordinary case for inputs that are not
+mine, and for spent identifiers under graph hiding — is not an error, and no delete-token
+event is published for it. A failure reported by the underlying store is always returned,
+regardless of whether the token was found locally, so that a transaction is never recorded
+as processed while its spends were not applied.
+
 
 ## Token Representations
 

token/services/tokens/storage.go

@@ -135,22 +135,22 @@ func NewTransaction(notifier events.Publisher, tx *tokendb.Transaction, tmsID to
 }
 
 // DeleteToken removes a single token from the database and notifies listeners.
+//
+// Delete is idempotent: marking an unknown token as spent is not an error, so a
+// failure returned by Delete always signals a real storage failure and is
+// propagated even when the token is not present in the local store. Swallowing
+// it there would let a lost spend be recorded as a processed transaction.
 func (t *DBTransaction) DeleteToken(ctx context.Context, tokenID token2.ID, deletedBy string) error {
 	tok, owners, err := t.Tx.GetToken(ctx, tokenID, true)
 	if err != nil {
 		return errors.WithMessagef(err, "failed to get token [%s]", tokenID)
 	}
 
-	err = t.Tx.Delete(ctx, tokenID, deletedBy)
-	if err != nil {
-		if tok == nil {
-			logger.DebugfContext(ctx, "nothing further to delete for [%s]", tokenID)
-
-			return nil
-		}
-
+	if err := t.Tx.Delete(ctx, tokenID, deletedBy); err != nil {
 		return errors.WithMessagef(err, "failed to delete token [%s]", tokenID)
 	}
+
+	// The token is not in the local store, so there are no owners to notify.
 	if tok == nil {
 		logger.DebugfContext(ctx, "nothing further to delete for [%s]", tokenID)
 

token/services/tokens/storage_test.go

@@ -168,6 +168,44 @@ func TestTransaction_DeleteToken_Error(t *testing.T) {
 	assert.Error(t, err)
 }
 
+// TestTransaction_DeleteToken_DeleteErrorWhenTokenAbsent checks that a Delete
+// failure is reported even when the token is not present in the local store,
+// which is the ordinary case for inputs that are not mine. Swallowing it would
+// let a transaction be recorded as processed while the spend was never applied.
+func TestTransaction_DeleteToken_DeleteErrorWhenTokenAbsent(t *testing.T) {
+	ctx := context.Background()
+	tmsID := token.TMSID{Network: "net", Channel: "ch", Namespace: "ns"}
+	mockTx := &mock.FakeTokenStoreTransaction{}
+
+	tx, err := tokens.NewTransaction(nil, &tokendb.Transaction{TokenStoreTransaction: mockTx}, tmsID)
+	require.NoError(t, err)
+
+	// token absent locally, but Delete hits a genuine storage failure
+	mockTx.GetTokenReturns(nil, nil, nil)
+	mockTx.DeleteReturns(assert.AnError)
+
+	err = tx.DeleteToken(ctx, token2.ID{TxId: "tx1", Index: 0}, "me")
+	assert.ErrorIs(t, err, assert.AnError)
+}
+
+// TestTransaction_DeleteToken_AbsentTokenIsNotAnError checks that deleting a
+// token that is not in the local store succeeds without notifying any owner.
+func TestTransaction_DeleteToken_AbsentTokenIsNotAnError(t *testing.T) {
+	ctx := context.Background()
+	tmsID := token.TMSID{Network: "net", Channel: "ch", Namespace: "ns"}
+	mockTx := &mock.FakeTokenStoreTransaction{}
+	pub := &mock.FakePublisher{}
+
+	tx, err := tokens.NewTransaction(pub, &tokendb.Transaction{TokenStoreTransaction: mockTx}, tmsID)
+	require.NoError(t, err)
+
+	mockTx.GetTokenReturns(nil, nil, nil)
+
+	require.NoError(t, tx.DeleteToken(ctx, token2.ID{TxId: "tx1", Index: 0}, "me"))
+	assert.Equal(t, 1, mockTx.DeleteCallCount())
+	assert.Equal(t, 0, pub.PublishCallCount())
+}
+
 func TestTransaction_SetSpendableBySupportedTokenTypes(t *testing.T) {
 	ctx := context.Background()
 	tmsID := token.TMSID{Network: "net", Channel: "ch", Namespace: "ns"}