Issue 2063 silent storage error fix

Signed-off-by: Effi-S <effi.szt@gmail.com>

Author: Effi-S

token/services/identity/role/registry.go

@@ -22,6 +22,51 @@ type WalletFactory interface {
 	NewWallet(ctx context.Context, id idriver.WalletID, role idriver.IdentityRoleType, is IdentitySupport, info idriver.IdentityInfo) (driver.Wallet, error)
 }
 
+// WalletIDStatus classifies the outcome of resolving an identity to the wallet id
+// bound to it. It exists so callers branch on an explicit, named state instead of
+// re-deriving intent from the ambiguous "(string, error)" shape — where ("", nil),
+// ("", err) and ("id", nil) each mean something different and the difference is easy
+// to get wrong (see issue #2063).
+type WalletIDStatus int
+
+const (
+	// WalletIDUnknown is the zero value and never returned by GetWalletID; it guards
+	// against a WalletIDResolution that was constructed without going through GetWalletID.
+	WalletIDUnknown WalletIDStatus = iota
+	// WalletIDBound means storage holds a wallet id for the identity. WalletID is set.
+	WalletIDBound
+	// WalletIDUnbound means storage answered authoritatively that the identity has no
+	// wallet binding. This is a definitive, successful miss: it is safe to fall through
+	// to the next resolution step and, ultimately, to create a wallet.
+	WalletIDUnbound
+	// WalletIDFailed means the storage lookup itself failed (timeout, connection reset,
+	// ...), so whether a binding exists is UNKNOWN. Err carries the cause. Callers MUST
+	// NOT treat this as WalletIDUnbound: doing so lets a transient blip masquerade as an
+	// unregistered identity and triggers the creation of a duplicate wallet.
+	WalletIDFailed
+)
+
+// WalletIDResolution is the explicit result of resolving an identity to its bound
+// wallet id. It is the single shared value every wallet-lookup fallback branches on,
+// so the "not found" vs "could not check" distinction is decided once (in GetWalletID)
+// rather than re-inferred at each call site.
+type WalletIDResolution struct {
+	// Status is the outcome of the lookup. Always inspect it via Bound/Unbound/Failed
+	// before reading the other fields.
+	Status WalletIDStatus
+	// WalletID is the bound wallet id; meaningful only when Status is WalletIDBound.
+	WalletID idriver.WalletID
+	// Err is the underlying storage failure; set only when Status is WalletIDFailed.
+	Err error
+}
+
+// Bound reports whether the identity has a wallet id bound to it.
+func (r WalletIDResolution) Bound() bool { return r.Status == WalletIDBound }
+
+// Failed reports whether the storage lookup failed, leaving the binding unknown.
+// A failed resolution must abort the enclosing lookup, never fall through to creation.
+func (r WalletIDResolution) Failed() bool { return r.Status == WalletIDFailed }
+
 // Registry manages wallets whose long-term identities have a given role.
 //
 // Concurrency and invariants:
@@ -82,16 +127,17 @@ func (r *Registry) Lookup(ctx context.Context, id driver.WalletLookupID) (driver
 		if ok {
 			r.Logger.DebugfContext(ctx, "lookup failed, check if there is a wallet for identity [%s]", passedIdentity)
 			// is this identity registered
-			wID, gwErr := r.GetWalletID(ctx, passedIdentity)
-			if gwErr != nil {
-				// A storage error must not be treated as "not registered": doing so would
-				// let a transient blip fall through to wallet creation and duplicate state.
-				return nil, nil, "", errors.WithMessagef(gwErr, "failed to lookup wallet [%s]", id)
+			res := r.GetWalletID(ctx, passedIdentity)
+			if res.Failed() {
+				// A storage failure leaves the binding unknown; it must not be treated as
+				// "not registered", or a transient blip would fall through to wallet
+				// creation and duplicate state.
+				return nil, nil, "", errors.WithMessagef(res.Err, "failed to lookup wallet [%s]", id)
 			}
-			if len(wID) != 0 {
-				r.Logger.DebugfContext(ctx, "lookup failed, there is a wallet for identity [%s]: [%s]", passedIdentity, wID)
+			if res.Bound() {
+				r.Logger.DebugfContext(ctx, "lookup failed, there is a wallet for identity [%s]: [%s]", passedIdentity, res.WalletID)
 				// we got a hit
-				walletID = wID
+				walletID = res.WalletID
 				ident = passedIdentity
 				fail = false
 			}
@@ -116,46 +162,48 @@ func (r *Registry) Lookup(ctx context.Context, id driver.WalletLookupID) (driver
 	if ok {
 		r.Logger.DebugfContext(ctx, "no wallet found, check if there is a wallet for identity [%s]", passedIdentity)
 		// is this identity registered
-		passedWalletID, err := r.GetWalletID(ctx, passedIdentity)
-		if err != nil {
-			// A storage error must not be treated as "not registered": doing so would
-			// let a transient blip fall through to wallet creation and duplicate state.
-			return nil, nil, "", errors.WithMessagef(err, "failed to lookup wallet [%s]", id)
+		res := r.GetWalletID(ctx, passedIdentity)
+		if res.Failed() {
+			// A storage failure leaves the binding unknown; it must not be treated as
+			// "not registered", or a transient blip would fall through to wallet
+			// creation and duplicate state.
+			return nil, nil, "", errors.WithMessagef(res.Err, "failed to lookup wallet [%s]", id)
 		}
-		if len(passedWalletID) != 0 {
-			r.Logger.DebugfContext(ctx, "no wallet found, there is a wallet for identity [%s]: [%s]", passedIdentity, passedWalletID)
+		if res.Bound() {
+			r.Logger.DebugfContext(ctx, "no wallet found, there is a wallet for identity [%s]: [%s]", passedIdentity, res.WalletID)
 			// we got a hit
 			r.WalletMu.RLock()
-			walletEntry, ok = r.Wallets[passedWalletID]
+			walletEntry, ok = r.Wallets[res.WalletID]
 			r.WalletMu.RUnlock()
 			if ok {
-				return walletEntry, nil, passedWalletID, nil
+				return walletEntry, nil, res.WalletID, nil
 			}
-			r.Logger.DebugfContext(ctx, "no wallet found, there is a wallet for identity [%s]: [%s] but it has not been recreated yet", passedIdentity, passedWalletID)
+			r.Logger.DebugfContext(ctx, "no wallet found, there is a wallet for identity [%s]: [%s] but it has not been recreated yet", passedIdentity, res.WalletID)
 		}
-		walletIdentifiers = append(walletIdentifiers, passedWalletID)
+		walletIdentifiers = append(walletIdentifiers, res.WalletID)
 	}
 
 	r.Logger.DebugfContext(ctx, "no wallet found for [%s] at [%s]", passedIdentity, logging.Prefix(wID))
 	if len(ident) != 0 {
-		identityWID, err := r.GetWalletID(ctx, ident)
-		r.Logger.DebugfContext(ctx, "wallet for identity [%s] -> [%s:%s]", ident, identityWID, err)
-		if err != nil {
-			// A storage error must not be treated as "not registered": doing so would
-			// let a transient blip fall through to wallet creation and duplicate state.
-			return nil, nil, "", errors.WithMessagef(err, "failed to lookup wallet [%s]", id)
+		res := r.GetWalletID(ctx, ident)
+		r.Logger.DebugfContext(ctx, "wallet for identity [%s] -> [%s:%d]", ident, res.WalletID, res.Status)
+		if res.Failed() {
+			// A storage failure leaves the binding unknown; it must not be treated as
+			// "not registered", or a transient blip would fall through to wallet
+			// creation and duplicate state.
+			return nil, nil, "", errors.WithMessagef(res.Err, "failed to lookup wallet [%s]", id)
 		}
-		if len(identityWID) != 0 {
+		if res.Bound() {
 			r.WalletMu.RLock()
-			w, ok := r.Wallets[identityWID]
+			w, ok := r.Wallets[res.WalletID]
 			r.WalletMu.RUnlock()
 			if ok {
-				r.Logger.DebugfContext(ctx, "found wallet [%s:%s:%s:%s]", ident, walletID, w.ID(), identityWID)
+				r.Logger.DebugfContext(ctx, "found wallet [%s:%s:%s:%s]", ident, walletID, w.ID(), res.WalletID)
 
-				return w, nil, identityWID, nil
+				return w, nil, res.WalletID, nil
 			}
 		}
-		walletIdentifiers = append(walletIdentifiers, identityWID)
+		walletIdentifiers = append(walletIdentifiers, res.WalletID)
 	}
 
 	for _, walletIdentifier := range walletIdentifiers {
@@ -248,21 +296,31 @@ func (r *Registry) GetIdentityMetadata(ctx context.Context, identity driver.Iden
 	return json.Unmarshal(raw, &meta)
 }
 
-// GetWalletID returns the wallet identifier bound to the passed identity.
+// GetWalletID resolves the wallet identifier bound to the passed identity.
 //
-// A storage error is propagated to the caller: an identity that has never been
-// bound is reported by the storage layer as ("", nil), so a non-nil error here
-// means a genuine storage failure (timeout, connection reset, ...). Callers must
-// not treat that failure as "no wallet bound", otherwise a transient blip would
-// look like an unregistered identity and trigger the creation of a duplicate wallet.
-func (r *Registry) GetWalletID(ctx context.Context, identity driver.Identity) (string, error) {
+// It is the single point that translates the storage layer's (WalletID, error)
+// convention into an explicit WalletIDResolution, so no caller has to re-derive the
+// meaning of ("", nil) vs ("", err). The storage contract reports an unbound identity
+// as ("", nil); a non-nil error is a genuine storage failure (timeout, connection
+// reset, ...) whose result is therefore WalletIDFailed, never WalletIDUnbound. Keeping
+// the two apart here is what prevents a transient blip from looking like an
+// unregistered identity and triggering the creation of a duplicate wallet (issue #2063).
+func (r *Registry) GetWalletID(ctx context.Context, identity driver.Identity) WalletIDResolution {
 	wID, err := r.Storage.GetWalletID(ctx, identity, int(r.Role.ID()))
 	if err != nil {
-		return "", errors.Wrapf(err, "failed to get wallet id for identity [%s]", identity)
+		return WalletIDResolution{
+			Status: WalletIDFailed,
+			Err:    errors.Wrapf(err, "failed to get wallet id for identity [%s]", identity),
+		}
+	}
+	if len(wID) == 0 {
+		r.Logger.DebugfContext(ctx, "no wallet bound to identity [%s]", identity)
+
+		return WalletIDResolution{Status: WalletIDUnbound}
 	}
 	r.Logger.DebugfContext(ctx, "wallet [%s] is bound to identity [%s]", wID, identity)
 
-	return wID, nil
+	return WalletIDResolution{Status: WalletIDBound, WalletID: wID}
 }
 
 func (r *Registry) WalletByID(ctx context.Context, role idriver.IdentityRoleType, id driver.WalletLookupID) (driver.Wallet, error) {

token/services/identity/role/registry_test.go

@@ -133,18 +133,33 @@ func TestBindIdentityAndContainsAndMetadataAndGetWalletID(t *testing.T) {
 	require.NoError(t, reg.GetIdentityMetadata(ctx, []byte("id"), "w", &meta))
 	require.Equal(t, "v", meta["k"])
 
-	// GetWalletID when storage returns value
+	// GetWalletID when storage returns a bound wallet id.
 	storage.GetWalletIDReturns("w", nil)
-	wid, err := reg.GetWalletID(ctx, []byte("id"))
-	require.NoError(t, err)
-	require.Equal(t, "w", wid)
-
-	// GetWalletID when storage returns error -> error is propagated (not suppressed),
-	// so a transient storage blip cannot masquerade as "no binding".
+	res := reg.GetWalletID(ctx, []byte("id"))
+	require.Equal(t, role.WalletIDBound, res.Status)
+	require.True(t, res.Bound())
+	require.NoError(t, res.Err)
+	require.Equal(t, "w", res.WalletID)
+
+	// GetWalletID when storage authoritatively reports no binding -> WalletIDUnbound,
+	// a clean miss with no error and no wallet id.
+	storage.GetWalletIDReturns("", nil)
+	resUnbound := reg.GetWalletID(ctx, []byte("id"))
+	require.Equal(t, role.WalletIDUnbound, resUnbound.Status)
+	require.False(t, resUnbound.Bound())
+	require.False(t, resUnbound.Failed())
+	require.NoError(t, resUnbound.Err)
+	require.Empty(t, resUnbound.WalletID)
+
+	// GetWalletID when storage fails -> WalletIDFailed with the error preserved, so a
+	// transient storage blip cannot masquerade as "no binding".
 	storage.GetWalletIDReturns("", errors.New("boom"))
-	wid2, err2 := reg.GetWalletID(ctx, []byte("id"))
-	require.Error(t, err2)
-	require.Empty(t, wid2)
+	resFailed := reg.GetWalletID(ctx, []byte("id"))
+	require.Equal(t, role.WalletIDFailed, resFailed.Status)
+	require.True(t, resFailed.Failed())
+	require.False(t, resFailed.Bound())
+	require.Error(t, resFailed.Err)
+	require.Empty(t, resFailed.WalletID)
 }
 
 // TestLookup_StorageErrorDoesNotFallThrough pins the fix for #2063: a transient