fix(identity): make RecipientDataCache provisioning cancellable - #2137
Open
AkramBitar wants to merge 3 commits into
Open
fix(identity): make RecipientDataCache provisioning cancellable#2137AkramBitar wants to merge 3 commits into
AkramBitar wants to merge 3 commits into
Conversation
AkramBitar
added
bug
AkramBitar
marked this pull request as draft
August 4, 2026 12:34
AkramBitar
marked this pull request as ready for review
August 4, 2026 13:51
RecipientDataCache started provisionIdentities as a bare goroutine under a sync.Once with a hardcoded context.Background(). It had no termination path: it lived for the process lifetime, pinned the cache, its channel and the backend closure, and blocked forever on the channel send once the cache filled up. Its error path was a bare continue, so a persistently failing identity backend turned it into a silent busy loop with nothing in the logs to explain it. TMSProvider.Update unloads the previous token management service on every public-parameter update, so each reload abandoned one such goroutine per anonymous owner wallet. The cache now builds a cancellable context in its constructor and exposes an idempotent Close(). The goroutine selects on ctx.Done() around the channel send, so cancellation releases it even when parked on a full cache, and it refuses to start at all if the cache was closed before its first use. Backend failures are logged and retried after a one second interruptible backoff instead of spinning, and the cache level gauge is incremented only once an entry has actually been handed over, so a cancelled send cannot skew it. Close() is wired into the existing teardown chain: AnonymousOwnerWallet.Close() forwards to the cache, and Registry.Done() closes every wallet it created that holds releasable resources before dropping the wallet map. The assertion uses a local closer interface rather than widening driver.Wallet, since only anonymous owner wallets have anything to release. Fixes #2122 Signed-off-by: AkramBitar <akram@il.ibm.com> Signed-off-by: Effi-S <effi.szt@gmail.com>
KeyManagerProvider.Get built an IdentityCache and kept only its Identity method value, discarding the cache object. The cache was therefore unreachable and its Close() had no callers, so its background provisioning goroutine could never be stopped — the very pattern the RecipientDataCache fix is modelled on was itself inert. WrappedKeyManager now retains the cache (nil for remote key managers, which do not pre-provision) and exposes Close(). LocalMembership tracks the key managers it loads and releases them in Close(), which is already reached by Role.Done(). Key managers are closed before the notifier teardown, since that block can return early or fail and must not skip the release. Addresses the remaining item of #2080. Signed-off-by: AkramBitar <akram@il.ibm.com> Signed-off-by: Effi-S <effi.szt@gmail.com>
…sioning failures Two follow-ups found while re-checking this branch against #2122 and #2080. The idemix identity cache assigned its cancel function inside sync.Once while Close read it unsynchronised. That was harmless as long as Close had no callers, but this branch gives it one, which makes it a live data race — confirmed by the race detector — with a worse consequence than the race itself: a Close that observed the pre-write nil value silently skipped the cancellation and leaked the goroutine anyway, which is the exact failure this branch is meant to remove. The context is now built in the constructor, so the field is write-once, and a cache closed before its first use no longer starts a goroutine at all. Its cache level gauge is also incremented only after a successful send, matching the other cache. Both caches now count failed provisioning attempts. #2080 notes that the busy loop had "no counter to signal the condition externally" and that there was "nothing to alert on"; a log line alone does not close that gap, since nothing pages on it. The new counters declare network/channel/namespace in LabelNames, as the TMS-wrapped provider binds those on every metric and a mismatch panics on first use. Existing idemix tests now release the key managers and caches they create. They were leaving provisioning goroutines running for the remainder of the test binary, which is what made goroutine assertions order-dependent. Signed-off-by: AkramBitar <akram@il.ibm.com> Signed-off-by: Effi-S <effi.szt@gmail.com>
Effi-S
force-pushed
the
fix/2122-recipient-data-cache-goroutine
branch
from
August 7, 2026 14:51
5b3e6ab to
60a24f7
Compare
Effi-S
approved these changes
Aug 9, 2026
| if c.cancel != nil { | ||
| c.cancel() | ||
| } | ||
| c.cancel() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both issues are about the same background worker, so they are fixed together in one PR.
Background. An anonymous wallet needs a fresh pseudonym for every payment. Making one is slow, so each wallet keeps a small stock of them ready. A background worker refills that stock. There is one worker per wallet. Idemix identities are cached the same way, by a second worker of the same shape.
#2122 — the worker cannot be stopped.
Once started, the worker runs until the process exits. There is no way to shut it down. When the stock is full, the worker waits to hand over the next pseudonym, and that wait never ends. So the worker stays alive forever, and it keeps its wallet, its stock, and everything they point to alive with it. Because wallets are created as they are used and never removed, the number of stuck workers grows with usage.
This PR gives the worker a proper shutdown path, so it can be told to stop and it actually exits.
#2080 — the same worker, plus three more problems.
A defect this PR had to fix in itself. The idemix cache prepared its cancellation while serving the first request, and read it back while shutting down, with no synchronisation between the two. That was harmless while nothing could call its shutdown method. Making it reachable turned it into a real race, with a worse effect than the race itself: a shutdown that arrived at the wrong moment silently did nothing and left the worker running — the very thing this PR sets out to fix. It is now prepared up front, once, so the two can no longer overlap.
New metrics.
recipient_data_provision_failures_totalandcache_provision_failures_total, so a failing identity backend can be alerted on instead of only appearing in the logs. Both declarenetwork/channel/namespace, and were checked against a real Prometheus provider behind the TMS wrapper, because a missing label there panics on first use.What the two issues share. Only the first point: giving the worker a way to shut down. Everything else is unique to #2080.
What is still not solved. Wallets are never removed once created. So a worker is only released when the whole token service is unloaded, which happens on a public-parameter update. During normal operation nothing releases them. Fixing that means deciding when a wallet is no longer needed, which is a separate change.
Testing. 16 new tests. 8 of them were checked against the old code first and do fail there, so they really catch these bugs. Existing tests that used to leave workers running now release them.
make checks,make lint,./token/...,-race, and repeated shuffled runs are all clean. Docs updated indocs/services/identity.md, including a table of the four cache metrics.Fixes #2122
Fixes #2080