feat(sherdlock): coordinate token-lock cleanup leadership across replicas

[sid200727]

Summary

Implements #1798: currently every replica runs the token-locks cleanup tick independently, causing wasted, overlapping work, all replicas race to delete the same expired rows at the same time. This adds a per-tick leadership check so only one replica performs cleanup, following the exact same pattern already established for keystore cleanup (AcquireCleanupLeadership).

Changes

-> Added AcquireCleanupLeadership to both driver.TokenLockStore and sherdlock.Locker interfaces
-> Added a shared driver.NoopCleanupLeadership for non-distributed backends (sqlite, in-memory), which always grant leadership locally, no failover code needed since there's only ever one instance
-> Wired the real Postgres advisory-lock implementation via the existing NewCleanupLeaderFactory(), using a lock id independent of the schema-creation lock id already used by TokenLockStore.lockID (deliberately separate to avoid any collision between the two purposes)
-> manager.go's cleaner() now acquires leadership before each Cleanup tick, skips the tick if not acquired, and releases leadership after, matching the design: per-tick election, no permanent leader, no failover code, advisory lock auto-releases if the winner crashes
-> Regenerated the FakeLocker mock; extended the hand-written mockLocker test double, defaulting to always-granted so all pre-existing tests are unaffected
-> Added two new subtests to TestManager_Cleaner: skip-on-no-leadership, and leadership-released-after-cleanup

Testing

Full test suite green (selector, sqlite, postgres).

Notes for the Reviewer

Followed the 4-step plan from the design discussion on #1798. Happy to add a Postgres integration test exercising two concurrent "replicas" if useful on top of the unit-level coverage already here, let me know if that's wanted before merge.

[AkramBitar]

LGTM.
See my comment

[AkramBitar]

Hello @sid200727,
Thanks a lot for submitting this PR. I added one comment.
Regarding the note in the description "Notes for the Reviewer"
It will be great to add such a test as described the question how much effort to add this test.

Regards,
Akram

[atharrva01]

The per-tick election looks right to me, and reusing NewCleanupLeaderFactory instead of adding a second lock primitive is the right call. One thing I think is a real problem though.

tokenLockCleanupLockID is a single global constant, but there's one Manager per TMS (SelectorService.managerLazyCache is keyed by *token.ManagementService). So a node running N TMSs against the same postgres ends up with N cleaner goroutines all fighting over the same advisory lock, even though their token_locks tables are completely separate. One TMS gets cleaned per tick and the rest just log "leadership not acquired" and skip. Right now they all clean fine every tick, so this makes multi-TMS nodes worse off. interop's topology wires 5 TMSs, so it's not a corner case.

I think the fix is to derive the id per store instead of passing it down from the manager. TokenLockStore already does this for schema creation with createTableLockID("tokenlock"), so something like createTableLockID(prefix + "tokenlock_cleanup") inside the postgres AcquireCleanupLeadership would make it per-TMS. You could then also drop the lockID param from the Locker interface, since sqlite and in-memory ignore it anyway.

Related: the keystore cleanup you're following takes its id from config (cleanup.Config.AdvisoryLockID, used at cleanup/manager.go:205). Here it's hardcoded, so if two deployments share a postgres instance there's no knob to fix it.

On the tests:

• leadership released after cleanup only checks that Close was called, not that Cleanup ran, or ran first. It would pass if the tick acquired leadership and released without cleaning anything.
• Neither new subtest calls m.Stop(), so both cleaner goroutines keep ticking every 50ms for the rest of the run, and CI runs the race detector. The trailing assert.NotNil(t, m) isn't really asserting anything either.
• The postgres test passes replicaA.lockID (424242, the schema lock) as the cleanup id, so it's exercising the wrong constant. It can't reach the real one without an import cycle, which I think is another sign the id is in the wrong package.

Minor: the advisory lock holds a dedicated conn off writeDB for the whole tick, so a cleanup now needs two write conns where it used to need one. Fine at MaxOpenConns: 10, just worth a comment since it's operator configurable.

Happy to be wrong on the first point if TMSs are normally expected to have separate data sources.