fix(selector): sherdlock per-attempt lock blacklist (Phase 3 of #2395) by adecaro · Pull Request #2398 · LFDT-Panurus/panurus

adecaro · 2026-09-21T11:40:12Z

Summary

Part of #2395. selectInternal had no memory of a lost lock race: after a
refetch, the same hot token could be re-proposed and re-lost repeatedly
within a single Select call — matching the incident's >6-minute
re-proposal loop on a single token.

This PR adds a per-call blacklist: every token this Select call has
already lost a lock race on is remembered and skipped on subsequent scans,
instead of being re-attempted and re-losing the same race. The blacklist is
scoped to the single Select call, not process-global, so a token
genuinely freed by another process is reconsidered on the caller's next
Select call. If an entire scan (since the last refetch) produces no
non-blacklisted candidate, the blacklist is cleared — otherwise a
genuinely-contended wallet with few tokens could turn a lost race into a
permanent false insufficient-funds.

This PR is stacked on #2397 (Phases 1-2: diagnostics + reproducible
contention baseline) and should be reviewed/merged after it.

Before / after (Phase 2 baseline, `sherdlock.TestHotTokenContention`)

metric	before (Phase 2 baseline)	after (this PR)
total lock attempts	7468	5096 (-32%)
total conflicts	7168	4796
conflict rate	0.96	0.94
distinct tokens attempted	300	300
max single-token conflict share	0.04	0.06

Total lock attempts drop substantially — far fewer wasted attempts against
tokens a call already knows it will lose. The aggregate conflict rate
barely moves because this fix only prevents re-attempts within a single
Select call, not across the many concurrent Select calls all racing for
the same rotating hot token; that cross-call contention is what Phase 4's
SQL anti-join and size-aware ordering address.

Test plan

go test ./token/services/selector/... (full suite, incl. new
TestHotTokenContention)
go test -race ./token/services/selector/sherdlock/...
make lint-auto-fix — 0 issues
make checks — pass (same pre-existing allow-listed GO-2024-3218)

Part of #2397

AkramBitar · 2026-09-22T10:29:22Z

@@ -206,17 +227,26 @@ func (s *Selector) selectInternal(ctx context.Context, owner token.OwnerFilter,

 			immediateRetries++


[Medium — blocker] A blacklist-only scan (every candidate was blacklisted, no TryLock called) still increments immediateRetries. On a wallet with a single hot token this means you burn 5 retries without ever winning a race — same DB refetch cost, half the chances. Fix: only increment immediateRetries when sawNonBlacklistedCandidate is true, or skip the increment on the blacklist-clear path.

AkramBitar · 2026-09-22T10:29:46Z

 			}
 			attempted.Add(t.Id)
+			sawNonBlacklistedCandidate = true
 			if errors.Is(lockErr, driver.ErrTokenAlreadyLocked) {


[Medium] (false, nil) from TryLock (contention without an error value) falls into the store-error else branch — logged as Warnf and never added to blacklisted. The blacklist only works when the locker wraps driver.ErrTokenAlreadyLocked. Document on the Locker interface that a lost race must return driver.ErrTokenAlreadyLocked, or treat !locked && lockErr == nil as a lost race here.

AkramBitar · 2026-09-22T10:30:04Z

.

AkramBitar

Reviewed by building and running the full selector suite on the PR head (passes), plus an instrumented head-vs-base comparison.

The mechanism is sound and not a no-op: tokenlock.go:91 maps UniqueKeyViolation to ErrTokenAlreadyLocked, locker.TryLock passes it through untouched, and the inmemory locker normalizes to the same sentinel, so the blacklist really does populate on both backends. The clear-on-full-exclusion guard correctly avoids turning a lost race into a false insufficient-funds, and the loop stays bounded.

Three inline comments. Two findings have no diff line to anchor to:

Test coverage. This PR touches only selector.go, and neither new behaviour is pinned by a test: that a token lost this call is skipped on the next scan, and that the blacklist clears when it excludes every candidate. The nearest existing test does not reach the new code - MaxRetriesExceeded stubs TryLockReturns(false, nil), so lockErr is nil, it routes to the store-error branch and never reaches blacklisted.Add. Separately, TestHotTokenContention states that Phases 3-5 should reduce the numbers it reports, but it only t.Logfs them, so the -32% in the description is enforced nowhere and can regress silently.

Docs. docs/services/selector.md documents exactly the semantics this changes - "a candidate already locked by another process is skipped and the loop moves on", the immediate-retry/backoff layer breakdown, and the sherdlock-vs-simple lock-retention contrast - and is not updated.

AkramBitar · 2026-09-22T10:35:25Z


+			if !sawNonBlacklistedCandidate && !blacklisted.Empty() {
+				s.logger.DebugfContext(ctx, "Blacklist excluded every candidate this scan; clearing it so freed tokens can be retried.")
+				blacklisted = collections.NewSet[token2.ID]()


Skip-only scans still consume the retry budget.

When a scan blacklists every candidate, the next scan skips them all, clears the blacklist here, and still falls through to immediateRetries++ (L228). So a scan that attempted no lock costs a retry.

Measured with a single always-contended token, same config both sides:

TryLock attempts refetches outcome

base (#2397) 6 6 SelectorSufficientButLockedFunds

this PR 3 6 same

Same six DB refetch round-trips and the same give-up point, but half as many chances to acquire the token. For a wallet whose only viable token is the hot one that lowers the success rate per unit of DB work, and part of the "-32% total lock attempts" figure is this rather than eliminated waste.

Not incrementing immediateRetries when the scan attempted nothing would keep the six real attempts while still preventing the back-to-back re-attempt this PR is about.

AkramBitar · 2026-09-22T10:35:25Z

+			}
+			sawNonBlacklistedCandidate = false
+
 			if immediateRetries > maxImmediateRetries {


No test asserts this boundary: that after maxImmediateRetries the selector returns SelectorSufficientButLockedFunds specifically. MaxRetriesExceeded (selector_test.go:151) stops at require.Error, with an outer budget of 1, so neither the error identity nor the 5-retry boundary is exercised.

Pre-existing, but worth pinning while the retry logic is being touched.

AkramBitar · 2026-09-22T10:35:25Z

 				// expected, common case under contention, not a DB error.
 				s.metrics.LockConflicts.Add(1)
 				s.logger.Infof("Lost lock race on token [%s:%d]: already locked by another process", t.Id.TxId, t.Id.Index)
+				blacklisted.Add(t.Id)


This line is reached only if the Locker wraps driver.ErrTokenAlreadyLocked.

A Locker that signals contention as (false, nil) - which the TryLock signature permits, and which this package's own test mocks do - falls into the else below, logs a nil error via Warnf, and is never blacklisted, so the fix silently does not apply to it.

Worth documenting on the Locker interface that a lost race must wrap the sentinel, or treating !locked && lockErr == nil as a lost race.

Part of #2395. Adds a read API for currently held token locks, a tokendiag locks CLI command built on it, and contention metrics plus structured logging in the sherdlock selector, so the next phases have a real baseline to measure against instead of guessing. - driver.TokenLockStore gains ListLocks; the sqlite/postgres common implementation is fixed to scan the shared TIMESTAMPTZ created_at column correctly on both dialects via a custom sql.Scanner (modernc.org/sqlite only auto-converts DATE/DATETIME/TIMESTAMP, not TIMESTAMPTZ). cond.NotExists is added to the query DSL as a prerequisite building block. - New cmd/tokendiag module with a `locks` subcommand that lists every held lock with its age and consumer tx status, and flags locks whose consumer has already reached a terminal status (Confirmed/Deleted/ Orphan) as leaked (mechanism 4 from the issue: nothing releases locks on settlement, so they sit until the next lease sweep). - sherdlock/metrics.go gains a LockConflicts counter (deliberately unlabeled by token/wallet id to avoid unbounded cardinality) and a DistinctTokensAttempted histogram per Select() call, to distinguish "one hot token retried many times" from "many tokens each contended once". - selector.go now distinguishes a lost lock race (driver.ErrTokenAlreadyLocked) from a genuine store error at the TryLock call site, promotes the conflict log line from Debug to Info with the token id, and the in-memory locker maps its own AlreadyLockedError to the same driver sentinel so both backends behave alike. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>

Part of #2395. Adds testutils.TestHotTokenContention, a workload shaped like the CERT incident (a few small tokens plus one large, rotating hot token, far more concurrent requests than tokens), and wires it in sherdlock/manager_test.go against real Postgres with a countingLocker decorator that records per-token lock attempts/conflicts. Baseline observed against 3 replicas x 100 requests: ~96% of lock attempts lose the race (conflict rate), across ~300 distinct rotating token IDs, with no single token ID ID absorbing a large share of conflicts, since deleteTokensAndStoreChange mints a fresh token ID each time the hot token is spent. The only hard assertion is the functional invariant that must hold regardless of contention distribution: total demand exactly equals total wallet balance, so no error can be a genuine insufficient-funds. This baseline is the yardstick for the fixes landing in phases 3-5 of #2395. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>

Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>

- selector: record a token as attempted once, before the TryLock outcome branches, so a rate-limited denial counts and a future third outcome branch cannot forget the call - selector: keep the trace on the lock-race and store-error lines with DebugfContext/WarnfContext, and stop the comment claiming the error split achieves more than a distinct log line - the caller still sees ordinary contention - tokendiag: drop the write-only Stores.db field and record on Close who owns the *sql.DB handle - tokendiag: trim the Run docstring, which promised a hot-token ranking separate from the age-sorted list the command actually prints - dbtest: cover the documented nil-Status case, a lock whose consumer has no requests row, which ListLocks' LEFT JOIN keeps visible - docs/metrics: match the widened attempted-token semantics and the debug-level lock-conflict line Signed-off-by: AkramBitar <akram@il.ibm.com>

… call Part of #2395. selectInternal previously had no memory of a lost lock race: after a refetch, the same hot token could be re-proposed and re-lost repeatedly by the same Select call, matching the incident's >6-minute re-proposal loop on a single token. Track tokens this call has already lost a race on in a per-call blacklist (scoped to the single Select invocation, not process-global, so a token genuinely freed by another process is reconsidered on the caller's next Select call) and skip them on subsequent scans instead of re-attempting the lock. If an entire scan since the last refetch produces no non-blacklisted candidate, the blacklist is cleared so a genuinely-contended wallet with few tokens is not turned into a permanent false insufficient-funds. Re-running the Phase 2 contention baseline (sherdlock.TestHotTokenContention) shows total lock attempts dropping from 7468 to 5096 (-32%), i.e. far fewer wasted attempts against tokens this call already knows it will lose. The aggregate conflict rate barely moves (0.96 -> 0.94) because this fix only prevents re-attempts within a single Select call, not across the many concurrent Select calls all racing for the same rotating hot token - that cross-call contention is addressed by the SQL anti-join and size-aware ordering in Phase 4. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>

adecaro · 2026-09-23T04:53:30Z

Superseded by #2410, which consolidates Phases 3-8 of #2395 into a single PR on top of #2397.

Consolidates the #2398-#2403 stack on top of #2397's diagnostics/baseline into a single PR against the same base: - Blacklist tokens that lost a lock race within a Select call (Phase 3) - Anti-join locked tokens and order candidates by amount ascending (Phase 4) - Release token locks as soon as a transaction reaches a terminal status, instead of waiting on the lease-expiry sweep (Phase 5) - Configurable Postgres lock-acquisition strategies: insert (default), onConflict, skipLocked, plus BatchLocker support for skipLocked (Phase 6) - Close 7 of 9 testable gaps in the lock-contention stack, plus two real bugs found along the way: LoadStorageConfig dropped already-parsed options on a later validation error, and selector.md documented an unreachable lease-expiry footgun (Phase 7) - Extend sherdlock benchmarks across cached/mixed fetcher strategies and the batch-locking path, and wire real metrics reporting into them (Phase 8) Fixes #2395 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>

The sherdlock token selector made a small number of "hot" tokens absorb the vast majority of lock collisions, surfacing as spurious "insufficient funds" errors under concurrent load (#2395). Six mechanisms contributed; this closes all of them, consolidating the #2398-#2403 stack (Phases 3-8) on top of #2397's diagnostics and baseline. Supersedes #2398, #2399, #2400, #2402, #2403. 1. Anti-join: the candidate query excludes already-locked tokens, so selectors stop queueing up to fight over the same row. 2. Amount-ordered candidates plus shuffle: tokens are fetched smallest-first so a small payment does not grab a large token, and equal-amount candidates are shuffled so contention does not simply shift onto whichever token sorts first. 3. Sufficiency-window randomization: once the ascending scan reaches a token that alone covers the remaining amount, a bounded lookahead picks uniformly among similarly-sized candidates - count-capped by sufficiencyWindow and magnitude-capped by maxSufficiencyRatio, anchored on the anchor token itself so the window does not collapse to size 1 when every candidate dwarfs the request. 4. Blacklisting: a token that lost a lock race is skipped for the rest of that Select call instead of being retried in a tight loop for minutes. 5. Immediate lock release on settlement: a transaction reaching a terminal status releases its locks through the finality listener and the recovery handler, rather than holding them until the lease-expiry sweep. Busy and Unknown are explicitly non-terminal and leave locks alone; OnError and a retry-exhausted OnStatus release exactly once. 6. Postgres lock strategies: onConflict and skipLocked avoid server-side unique-constraint violations on lost races, and LockBatch claims a covering window of candidates in one round trip. Review follow-ups folded in: the fast-fail balance check compares against the full requested quantity rather than the remaining amount, which it had been double-counting; a batch-lock store error refetches the window instead of silently dropping it, bounded by the existing retry budget; the EVM recovery handler releases selection locks like its Fabric counterpart; the now-unused HasAnySpendableTokens is removed from the driver interface and its implementations; and benchmark_test.go's pre-existing ireturn/thelper lint breakage is fixed. Two accuracy fixes from the last review round: StaleCandidates is incremented on the single-token lock path only - LockBatch reports just the tokens it won, so a stale candidate is indistinguishable there from a lost race and is booked as LockConflicts - which the counter's own documentation and the metrics page had claimed otherwise, and the batch branch now records the three consequences that follow from it. maxSufficiencyRatio also gains deterministic coverage: in both sufficiency-window tests the count cap binds first, so the ratio bound could be disabled without either of them noticing. Tests: hot-token contention suites reproducing the CERT incident's Pareto shape and its static-hot-token variant, a simple-driver baseline, lock-outcome classification across both the single-token and batch paths, stale-candidate handling on both, the sufficiency-window and ratio-boundary ordering tests, and finality listener/recovery coverage for every status transition that touches locks. Signed-off-by: AkramBitar <akram@il.ibm.com>

The sherdlock token selector made a small number of "hot" tokens absorb the vast majority of lock collisions, surfacing as spurious "insufficient funds" errors under concurrent load (#2395). Six mechanisms contributed; this closes all of them, consolidating the #2398-#2403 stack (Phases 3-8) on top of #2397's diagnostics and baseline. Supersedes #2398, #2399, #2400, #2402, #2403. 1. Anti-join: the candidate query excludes already-locked tokens, so selectors stop queueing up to fight over the same row. 2. Amount-ordered candidates plus shuffle: tokens are fetched smallest-first so a small payment does not grab a large token, and equal-amount candidates are shuffled so contention does not simply shift onto whichever token sorts first. 3. Sufficiency-window randomization: once the ascending scan reaches a token that alone covers the remaining amount, a bounded lookahead picks uniformly among similarly-sized candidates - count-capped by sufficiencyWindow and magnitude-capped by maxSufficiencyRatio, anchored on the anchor token itself so the window does not collapse to size 1 when every candidate dwarfs the request. 4. Blacklisting: a token that lost a lock race is skipped for the rest of that Select call instead of being retried in a tight loop for minutes. 5. Immediate lock release on settlement: a transaction reaching a terminal status releases its locks through the finality listener and the recovery handler, rather than holding them until the lease-expiry sweep. Busy and Unknown are explicitly non-terminal and leave locks alone; OnError and a retry-exhausted OnStatus release exactly once. 6. Postgres lock strategies: onConflict and skipLocked avoid server-side unique-constraint violations on lost races, and LockBatch claims a covering window of candidates in one round trip. Review follow-ups folded in: the fast-fail balance check compares against the full requested quantity rather than the remaining amount, which it had been double-counting; a batch-lock store error refetches the window instead of silently dropping it, bounded by the existing retry budget; the EVM recovery handler releases selection locks like its Fabric counterpart; the now-unused HasAnySpendableTokens is removed from the driver interface and its implementations; and benchmark_test.go's pre-existing ireturn/thelper lint breakage is fixed. Two accuracy fixes from the last review round: StaleCandidates is incremented on the single-token lock path only - LockBatch reports just the tokens it won, so a stale candidate is indistinguishable there from a lost race and is booked as LockConflicts - which the counter's own documentation and the metrics page had claimed otherwise, and the batch branch now records the three consequences that follow from it. maxSufficiencyRatio also gains deterministic coverage: in both sufficiency-window tests the count cap binds first, so the ratio bound could be disabled without either of them noticing. Tests: hot-token contention suites reproducing the CERT incident's Pareto shape and its static-hot-token variant, a simple-driver baseline, lock-outcome classification across both the single-token and batch paths, stale-candidate handling on both, the sufficiency-window and ratio-boundary ordering tests, and finality listener/recovery coverage for every status transition that touches locks. build(tools): bump staticcheck to v0.8.1 so make checks runs under Go 1.27 staticcheck v0.7.0 panics in its own IR builder (unexpected expr: *ast.KeyValueExpr) against this module's Go 1.27.1 toolchain, on packages unrelated to this change, which makes the checks-heavy stage of make checks unrunnable locally. v0.8.1 (2026.2.1) analyses the same tree cleanly with no new findings. Separable from the selector fix if a maintainer prefers it on its own. Signed-off-by: AkramBitar <akram@il.ibm.com>

adecaro added this to the Q3/26 milestone Sep 21, 2026

adecaro added bug Something isn't working performance db storage token-selector labels Sep 21, 2026

adecaro self-assigned this Sep 21, 2026

adecaro mentioned this pull request Sep 21, 2026

fix(selector): anti-join locked tokens and order candidates by amount #2399

Closed

4 of 5 tasks

adecaro added this pull request to stack #2401 September 21, 2026 14:52

adecaro mentioned this pull request Sep 22, 2026

sherdlock: add lock-contention diagnostics and a reproducible baseline test (Phases 1-2 of #2395) #2397

Merged

8 tasks done

AkramBitar reviewed Sep 22, 2026

View reviewed changes

AkramBitar requested changes Sep 22, 2026

View reviewed changes

adecaro and others added 4 commits September 22, 2026 16:58

fix documentation

5946b50

Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>

adecaro force-pushed the fix/2395-sherdlock-lock-contention-diagnostics branch from 6d8d3f4 to 58cde2e Compare September 22, 2026 14:58

adecaro mentioned this pull request Sep 22, 2026

Prefer exact-sum token combinations in selector to avoid unnecessary change outputs #2404

Open

adecaro mentioned this pull request Sep 22, 2026

test(selector): close lock-contention testable gaps + extend sherdlock benchmarks (Phases 7-8, #2395) #2403

Closed

8 of 9 tasks

adecaro force-pushed the fix/2395-sherdlock-per-attempt-blacklist branch from b5d9524 to e691fb5 Compare September 22, 2026 15:20

adecaro force-pushed the fix/2395-sherdlock-lock-contention-diagnostics branch from 58cde2e to d26ce8e Compare September 22, 2026 15:25

adecaro mentioned this pull request Sep 23, 2026

fix(selector): close sherdlock lock-contention gaps (Phases 3-8, #2395) #2410

Open

5 tasks done

adecaro closed this Sep 23, 2026

adecaro deleted the fix/2395-sherdlock-per-attempt-blacklist branch September 23, 2026 04:57

adecaro removed this pull request from stack #2401 September 23, 2026 05:08

This was referenced Sep 24, 2026

Prefer exact-sum token combinations in selector to avoid unnecessary change outputs Part: k=1 #2415

Open

Prefer exact-sum token combinations in selector to avoid unnecessary change outputs Part: K=2 #2421

Open

Navigation Menu

fix(selector): sherdlock per-attempt lock blacklist (Phase 3 of #2395) - #2398

fix(selector): sherdlock per-attempt lock blacklist (Phase 3 of #2395)#2398
adecaro wants to merge 5 commits into
fix/2395-sherdlock-lock-contention-diagnosticsfrom
fix/2395-sherdlock-per-attempt-blacklist

adecaro commented Sep 21, 2026 •

edited

Loading

Uh oh!

AkramBitar Sep 22, 2026

Uh oh!

AkramBitar Sep 22, 2026

Uh oh!

AkramBitar commented Sep 22, 2026 •

edited

Loading

Uh oh!

AkramBitar left a comment

Uh oh!

AkramBitar Sep 22, 2026

Uh oh!

AkramBitar Sep 22, 2026

Uh oh!

AkramBitar Sep 22, 2026

Uh oh!

adecaro commented Sep 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -206,17 +227,26 @@ func (s *Selector) selectInternal(ctx context.Context, owner token.OwnerFilter,

		immediateRetries++

	TryLock attempts	refetches	outcome
base (#2397)	6	6	`SelectorSufficientButLockedFunds`
this PR	3	6	same

Conversation

adecaro commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Before / after (Phase 2 baseline, sherdlock.TestHotTokenContention)

Test plan

Uh oh!

AkramBitar Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

AkramBitar Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

AkramBitar commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkramBitar left a comment

Choose a reason for hiding this comment

Uh oh!

AkramBitar Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

AkramBitar Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

AkramBitar Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

adecaro commented Sep 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adecaro commented Sep 21, 2026 •

edited

Loading

Before / after (Phase 2 baseline, `sherdlock.TestHotTokenContention`)

AkramBitar commented Sep 22, 2026 •

edited

Loading