Prevent Concurrent TokenLocks Cleanup Across Replicas

[AkramBitar]

Currently, each replica runs the TokenLocks cleanup function independently. We should investigate whether we can introduce a mechanism so that only a single replica performs this task at a time, in order to avoid contention

[sid200727]

Hi @AkramBitar, interesting one. I'd be interested in taking a look. Before claiming, I'd like to scope it properly since the complexity isn't pinned down yet. A few questions:

-> Backend scope: Is this expected to work for both Postgres and SQLite, or is Postgres-only acceptable? Postgres advisory locks (pg_advisory_lock / pg_try_advisory_lock) would be the natural primitive, but SQLite would need a different approach.
-> Existing primitives: Is there an existing distributed-coordination primitive in panurus or FSC I should reuse rather than introducing a new mechanism? The closest I've seen is the recovery claim leasing in token/services/storage/db/sql/postgres/recovery_claim*.go — that pattern could potentially extend here.
-> Election granularity: Should "only one replica runs cleanup" be per-cleanup-cycle (any replica can win the race each interval, all equal) or process-lifetime (one leader for the duration of a process)? Trade-off is fairness vs. stability.

Happy to draft a short design proposal as a comment here once these are clear, then claim and implement.

[AkramBitar]

Hi @sid200727 thanks a lot for looking at this issue. I assigned it to you.

Answering your questions:

1. Backend scope: Works for both. Each backend plugs in its own leader factory; Postgres uses a real advisory lock, SQLite/memory get an always-granted no-op (fine, not shared across replicas).

• https://github.com/hyperledger-labs/fabric-token-sdk/blob/main/token/services/storage/db/sql/common/tokens.go#L1183

2. Existing primitive: Reuse the existing pattern — acquire leadership, skip if you didn't win, release after. The keystore cleanup already does exactly this, and we'd apply the same shape to the TokenLocks cleaner.

Maybe to copy: https://github.com/hyperledger-labs/fabric-token-sdk/blob/main/token/services/storage/services/cleanup/manager.go#L203
to https://github.com/hyperledger-labs/fabric-token-sdk/blob/main/token/services/selector/sherdlock/manager.go#L86

3. Granularity: Today every replica cleans up at the same time, so they all try to delete the same rows at once, and this is a wasted, overlapping work. Instead, each tick they race for one shared lock: the winner cleans up and releases it, the rest skip. If the winner crashes, its lock auto-releases and another takes over next tick. No permanent leader, no failover code.

• https://github.com/hyperledger-labs/fabric-token-sdk/blob/main/token/services/storage/db/sql/postgres/advisorylock.go#L80

Hope this answered your questions.

Thanks a lot,
Akram

[sid200727]

Thank you @AkramBitar 🙏
This is really clear, especially the keystore cleanup pattern reference and the advisory lock approach. The per-tick election with auto-release on crash is exactly the right shape (no permanent leader = no failover code = much simpler).
I'll study the three referenced files (cleanup/manager.go, sherdlock/manager.go, advisorylock.go) and draft a short design proposal here before opening a PR, just to make sure my approach matches your intent.

Will have the design proposal up soon.

[sid200727]

Studied the three references, this maps directly onto the existing keystore-cleanup leadership pattern, and reuses infrastructure that already exists rather than adding new locking code:

postgres.NewCleanupLeaderFactory() already returns a driver.CleanupLeadership backed by pg_try_advisory_lock/pg_advisory_unlock, and AdvisoryLock already satisfies that interface. No new Postgres-side locking code needed.
Proposal: add an AcquireCleanupLeadership(ctx, lockID) (CleanupLeadership, bool, error) method to the sherdlock.Locker interface, wired per-backend the same way TokenStore.cleanupLeaderFactory is wired today:

Postgres TokenLockStore: plug in NewCleanupLeaderFactory(), using a distinct lock ID from the schema-init lock (e.g. createTableLockID("tokenlock_cleanup")) so the two don't collide.
SQLite/inmemory TokenLockStore: return noopCleanupLeadership{}, true, nil (always granted) matches how the keystore cleanup already handles non-distributed backends.

In sherdlock.Manager.cleaner(), before calling m.locker.Cleanup(...), acquire leadership; skip the tick if not acquired; defer leadership.Close() on success. Same shape as cleanup.Manager.runSweep().
Per-tick election, as you described: no permanent leader, no failover code. If the winner dies mid-cleanup, the advisory lock auto-releases on connection close and the next tick's winner takes over.

Plan to implement as:
(1) interface + no-op backends
(2) Postgres advisory-lock wiring
(3) manager.go leadership check
(4) unit tests using the existing mockLocker/FakeLocker pattern plus a Postgres itest exercising two concurrent "replicas."

Will open the PR against this shape unless you'd prefer a different lock-ID scheme or leadership granularity.

[sid200727]

Hi @AkramBitar, please let me know if you agree with the proposal.
Sorry for the late reply😅
Thank You