test(selector): close lock-contention testable gaps + extend sherdlock benchmarks (Phases 7-8, #2395) by adecaro · Pull Request #2403 · LFDT-Panurus/panurus

adecaro · 2026-09-22T10:48:01Z

Summary

Stacked on #2402. Phases 7–8 of #2395.

Phase 7 — close testable gaps in the lock-contention stack

Closes 7 of the 9 testable gaps identified in the PR-by-PR analysis
(docs/development/2395-lock-contention-analysis.md, posted on #2397). Not a
mechanism fix: the five mechanisms named in the issue were already closed by
Phases 3–6. This phase closes the coverage gaps those phases left behind,
plus two real bugs exploration turned up along the way.

Settlement release, contention-level (gap 1) + lock-age assertion (gap 2).
New TestHotTokenContentionWithSettlement routes the harness's release step
through a real finality.SelectorManagerProvider chain and a real Postgres
TokenLockStore, then asserts ListLocks is empty after every request
settles — closing the exact blind spot the CERT report raised about
f8a27fc4... staying locked well after settlement. driver.IsTerminalStatus
promoted out of cmd/tokendiag so TestReleaseAfterConfirm/
TestListLocksReportsLockAge (new dbtest cases, all three drivers for
free) can assert directly on it. selector_manager_test.go also adds the
provider's first test (error path, nil, nil, no-caching).
Size-ordering regression test (gap 3). New sherdlock/ordering_test.go
reproduces the exact "1 EUR request shouldn't draw the 200 EUR token"
scenario over a real lazyFetcher + MockQueryService, container-free.
Found and fixed two real bugs along the way: MockQueryService.WarmupCache's
non-deterministic map iteration (didn't match the real fetcher's
ORDER BY amount ASC), and a nil-dereference panic in
SpendableTokensIteratorBy's collections.Map transformer on a full drain
(also affects the benchmarks, unnoticed because they don't run under
go test).
Lock-failure classification (gap 4). New
sherdlock/lock_classification_test.go covers the batch-path rate-limit
hard-abort (previously unexercised), the deliberate store-error-vs-lost-race
asymmetry in both the batch and single-token paths, the blacklist-clearing
escape hatch, and (closing gap 9) LockConflicts/DistinctTokensAttempted
metrics — asserting exactly when each fires and, just as important, when it
must not.
Rolling-deploy mixed-strategy test (gap 6). New
postgres/tokenlock_mixed_strategy_test.go races an insert-strategy
replica against a skipLocked-strategy replica over the same token/table —
the scenario Phase 6's docs claimed was safe but never tested.
leaseExpiry: 0 (gap 7) — turned out to be a doc bug, not a reachable
misconfiguration. Config.GetLeaseExpiry()/GetLeaseCleanupTickPeriod()
already coerce 0 to the default, so YAML can never actually disable the
sweep. Docs corrected; sherdlock.NewManager now warns on the one
in-process path that can still hit it.

Bug found and fixed in passing: common.LoadStorageConfig returned a
zero-value StorageConfig on any validation error, so a typo'd
lockStrategy silently discarded an already-successfully-parsed
TableNames/SkipPrefix at the warn-and-continue call sites in the
postgres/sqlite drivers. Now returns every option parsed successfully so far
alongside the error.

Excluded from this phase (user directive): the simple-driver contention
baseline (gap 5) and the fabtoken/dlog integration tests (part of gap 8); a
fuzz target for the config parser was also considered and skipped.

Phase 8 — extend the selector benchmarks across sherdlock's options, make metrics observable

benchmark_test.go previously only exercised the lazy fetcher and
single-token locking, and always wired sherdlock.Metrics to
disabled.Provider — every counter/histogram sherdlock records
(LockConflicts, SelectionOutcome, ImmediateRetries,
DistinctTokensAttempted, SelectionDuration, UnspentTokensInvocations)
was silently discarded, so a benchmark run could never show why one setting
was faster or slower than another.

New benchMetricsProvider — an in-memory fscmetrics.Provider that
records every observation (mirroring the real prometheus provider's
label-pair With() convention) instead of discarding it. Wired into every
sherdlock-backed provider function via a single shared sherdlock.Metrics
per selector, and reported through reportSherdlockMetrics in both
BenchmarkSelectorSingle and BenchmarkSelectorParallel's b.Run
closures, so go test -v -bench output now shows real numbers instead of
nothing.
New fetcher-strategy settings — sherdlock+cached (eager/cached
snapshot, NewCachedFetcher) and sherdlock+mixed (try-eager-then-lazy,
NewMixedFetcher), alongside the existing lazy-fetcher settings, so all
three FetcherStrategy code paths are benchmarked head to head.
New batch-locking settings — sherdlock+batchlock and
sherdlock+batchlock+contention, via a new benchBatchLocker wrapper
(every production BatchLocker is Postgres-specific, so nothing in-memory
could exercise selectInternal's batch-locking branch before this).
Bug found and fixed along the way:
testutils.MockQueryService.SpendableTokensIteratorBy didn't replicate
production's "empty walletID/type = no filter" semantics
(HasTokenDetails in tokens.go). sherdlock's cached fetcher scans the
whole DB via SpendableTokensIteratorBy(ctx, "", ""); against the
unfixed mock this always found zero tokens, so the new
sherdlock+cached setting failed every selection (locked/insufficient
funds) until this was fixed.
Deliberately out of scope: Postgres lock-acquisition strategies
(insert/onConflict/skipLocked) — Postgres-specific, already covered
by sherdlock/contention_test.go's integration-style tests; rate-limiting
via ratelimit.Decorate — wraps SelectorManager, not a raw Selector,
and would need an adapter not currently justified by this benchmark's
scope.

Numbers, for reference

TestHotTokenContention (3 replicas × 100 CHF1 requests), Phase 2 → Phase 4:
total lock attempts 7468 → 3414 (−54%), total conflicts 7168 → 3114 (−57%),
distinct tokens actually contended 300 → 212. Phase 6 strategy comparison
(TestHotTokenContention_SingleTokenLockPath, unique-constraint violations):
insert = thousands per run, onConflict/skipLocked = 0. Phase 7's new
settlement test: 300 requests, 0 spurious errors, 0 locks remaining after
settlement.

Phase 8 benchmark highlights (-benchtime 200x, single 1M-token wallet
unless noted; contended settings use 8 clients over a 1000-token wallet):

Setting	ns/op	selects/sec
sherdlock+batchlock	59,660	16,762
sherdlock	73,106	13,679
sherdlock+mixed	74,370	13,446
sherdlock+cached	82,502	12,121
sherdlock+lock	101,643	9,838
sherdlock+lock+contention	871,145	1,148
sherdlock+lock+contention+stubborn (backoff)	1,524,316	656
sherdlock+batchlock+contention	615,990	1,623

Batch-locking is ~30% faster than single-token locking under identical
contention (615,990 ns/op vs 871,145 ns/op). The backoff-retrying
("stubborn") variant traded ~1.75x latency for resolving 100% of contention
(0/200 failures vs 1/200 without backoff).

Test plan

make lint-auto-fix clean (0 issues, all 10 Go modules)
make checks clean (only the pre-existing allow-listed libp2p
govulncheck finding, unrelated)
go test ./token/services/storage/db/... (memory/sqlite/postgres)
go test ./token/services/selector/... and
go test -race ./token/services/selector/sherdlock/...
go test ./token/services/ttx/finality/...
go test ./token/services/storage/db/sql/postgres/ -run 'TokenLockStore'
cmd/tokendiag module builds and tests clean (separate go.mod)
go vet ./token/services/selector/... and
go test ./token/services/selector/... -bench 'BenchmarkSelector'
(Phase 8 benchmarks + metrics reporting, all settings pass)
Integration tests (fabtoken/dlog T1) — excluded from this phase per user
directive

Fixes #2398

…#2395) Stacked on #2402. Closes 7 of the 9 testable gaps identified in the PR-by-PR analysis (docs/development/2395-lock-contention-analysis.md): settlement release now has a contention-level regression test over a real finality.SelectorManagerProvider chain and real Postgres store, plus a lock-age assertion; size-ordered selection and lock-failure classification (batch and single-token paths, including the blacklist-clearing escape hatch) get dedicated tests; a rolling-deploy mixed-lock-strategy scenario is now exercised against real Postgres; LockConflicts/DistinctTokensAttempted finally have assertions. Excluded by explicit user directive: a `simple` driver baseline (gap 5) and the fabtoken/dlog integration tests (part of gap 8); a fuzz target for the config parser was also considered and skipped. Two real issues surfaced during this work and are fixed here, not just pinned as tests: - common.LoadStorageConfig returned a zero-value StorageConfig on any validation error, so a typo'd lockStrategy silently discarded an already-successfully-parsed TableNames/SkipPrefix at the warn-and-continue call sites in the postgres/sqlite drivers. Now returns every option parsed successfully so far alongside the error. - docs/services/selector.md documented `leaseExpiry: 0` / `leaseCleanupTickPeriod: 0` as an unguarded footgun that disables the lease sweep. It isn't reachable from YAML: Config.GetLeaseExpiry()/ GetLeaseCleanupTickPeriod() already coerce 0 to the default. Docs corrected; sherdlock.NewManager now warns on the one in-process path that can still hit it. Also found and fixed two bugs in the test harness itself along the way: MockQueryService.WarmupCache's non-deterministic map iteration (didn't match the real fetcher's ORDER BY amount ASC), and a nil-dereference panic in SpendableTokensIteratorBy's collections.Map transformer on a full drain (also affected the benchmarks, unnoticed because they don't run under `go test`). Verified: make lint-auto-fix (0 issues, all 10 modules), make checks, full storage/db + selector + sherdlock (-race) + ttx/finality + postgres TokenLockStore suites, and the separate cmd/tokendiag module — all pass, no regressions. Fixes #2395 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>

…options (Phase 8, #2395) Add benchmark settings exercising sherdlock's eager/cached and mixed fetcher strategies (NewCachedSherdSelector, NewMixedSherdSelector) and its batch-locking path (NewBatchLockSherdSelector + benchBatchLocker), none of which the existing benchmarks touched. Replace the disabled.Provider metrics sink with benchMetricsProvider, an in-memory fscmetrics.Provider that actually records observations, and wire reportSherdlockMetrics into both benchmark functions so sherdlock.Metrics (lock conflicts, selection outcome, immediate retries, distinct tokens attempted, selection duration, fetcher invocations) shows up in `go test -v` output instead of being silently discarded. Fix testutils.MockQueryService.SpendableTokensIteratorBy to treat an empty walletID/type as "no filter", matching production's HasTokenDetails semantics. Without this, sherdlock's cached fetcher (which scans the whole DB via SpendableTokensIteratorBy(ctx, "", "")) found zero tokens against the mock, so the new cached-fetcher setting failed every selection until this was fixed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>

AkramBitar

Review of the harness/test additions. The substance is good — the collections.Map nil guard in SpendableTokensIteratorBy is a genuine crash fix (iterators.Map does run the transformer on the exhaustion zero value, so a full drain panicked before), and the container-free tests plus the gap docs are worth having.

Three things I'd want fixed before merge, all in the new benchmark harness. Together they mean the contention settings don't actually contend, and the selection regression the harness was built to catch no longer fails the run — so the head-to-head numbers in the PR description shouldn't be relied on yet. Details inline.

Everything else I looked at held up: benchBatchLocker.LockBatch really does satisfy sherdlock.BatchLocker (transaction.ID is an alias, so the batch branch is taken), NoBackoff = -1 does route to StubbornSelector so +stubborn is not a no-op, the require.Len(t, replicas, 3) guards match every caller, the token mixes sum to the stated balances, and cmd/tokendiag builds and vets clean after the driver.IsTerminalStatus promotion.

AkramBitar · 2026-09-22T15:47:01Z

+	if u, ok := s.Selector.(interface {
+		UnlockAll(ctx context.Context) error
+	}); ok {
+		if err := u.UnlockAll(context.Background()); err != nil {


UnlockAll resolves to locker.UnlockByTxID(l.txID), and every setting builds exactly one Selector in setup() bound to the single constant testutils.TxID — which BenchmarkSelectorParallel then shares across all RunParallel goroutines.

So each iteration's Unselect clears every lock held under "someTxID", including the tokens other in-flight iterations are still holding. For sherdlock+lock+contention, +contention+stubborn and +batchlock+contention (8 clients over 1000 tokens) the lock table is repeatedly wiped by unrelated goroutines, so those settings aren't measuring the contention they report — which is what the "batch-locking ~30% faster under identical contention" figure in the PR body rests on.

A per-iteration selector (or per-iteration txID), or unlocking just the IDs that Select returned, would make this measure what it claims.

AkramBitar · 2026-09-22T15:47:01Z

 				ids, _, err := s.selector.Select(b.Context(), s.filter, testutils.SelectQuantity, testutils.TokenType)
-				if err != nil {
-					b.Error("unexpected error", err)
+				if isContended, isUnexpected := classifyOutcome(err); isUnexpected {


classifyOutcome treats SelectorInsufficientFunds / SelectorSufficientButLockedFunds as benign contention unconditionally, but BenchmarkSelectorSingle is a single goroutine against a wallet holding NumTokensPerWallet (1,000,000) unit tokens asking for 100. Neither error is ever legitimate in that configuration, yet the previous b.Error("unexpected error") is now just a counter and a b.Logf.

That's precisely the failure this PR describes hitting — sherdlock+cached "failed every selection (locked/insufficient funds)" against the unfixed mock. After this change that regression only produces a log line, so the next one like it will pass CI silently. Suggest keeping the hard failure for the uncontended clients: 1 settings and only softening it where clients > 1.

AkramBitar · 2026-09-22T15:47:01Z

+		// build its whole-DB snapshot (token/services/selector/sherdlock/fetcher.go); without
+		// this branch it always finds zero tokens, since q.cache is only ever warmed under a
+		// specific wallet key (see WarmupCache), never under "".
+		it = &token.UnspentTokensIterator{UnspentTokensIterator: &MockIterator{q, q.allKeys, 0}}


This whole-DB branch returns q.allKeys in insertion order, while WarmupCache is changed in this same PR to sort ascending by amount — specifically to honour the "items already ordered ascending by amount" precondition of newBucketedIterator / cachedFetcher.updateCache (see the comment at sherdlock/fetcher.go:450-455).

cachedFetcher.update reaches the store only through SpendableTokensIteratorBy(ctx, "", ""), i.e. exactly this branch, so the new sherdlock+cached and sherdlock+mixed settings get unsorted candidates. It passes today only because every benchmark token has amount 1; the Xavier 1 EUR / 200 EUR scenario from ordering_test.go would silently fail to reproduce smallest-fit if run through the cached or mixed fetcher. Worth sorting here too so the two paths agree.

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

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>

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. Third review round (review 5404492139 on #2410): the simple driver's bounded-pool deadlock is now fixed rather than documented as a known limitation. selectByID held its unspentTokens cursor open across the nested concurrencyCheck query, so every in-flight Select pinned two connections and a pool smaller than the concurrent-selector count deadlocked outright - each connection handed to an open cursor, each goroutine blocked waiting for a second one. The candidate scan is already finished with the cursor by then and a retry opens a fresh one, so it is closed before the re-check and one selection needs one connection. TestSimpleDriverBoundedPool pins it with 16 concurrent selectors against a pool of 2, and stalls if the overlapping checkout is restored. The same round adds idx_spendable_amount on (owner_wallet_id, token_type, amount), partial on the spendable-tokens query's own is_deleted/owner/spendable predicates, so its ORDER BY amount reads rows already ordered instead of sorting the wallet - a new index name rather than an extra column on idx_owner_wallet_part, since CREATE INDEX IF NOT EXISTS would not replace an index already deployed under that name. Plus the documented assumption behind bucketedIterator's bucket boundaries: they use string equality on the stored quantity, which identifies equal amounts only because that encoding is canonical while the ORDER BY is numeric, so a non-canonical encoding would degrade the shuffle to a no-op rather than produce a wrong order. IsTerminalStatus is left exported - cmd/tokendiag/cobra/locks/runner.go is a production caller, so it is not test-only. 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>

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. Third review round (review 5404492139 on #2410): the simple driver's bounded-pool deadlock is now fixed rather than documented as a known limitation. selectByID held its unspentTokens cursor open across the nested concurrencyCheck query, so every in-flight Select pinned two connections and a pool smaller than the concurrent-selector count deadlocked outright - each connection handed to an open cursor, each goroutine blocked waiting for a second one. The candidate scan is already finished with the cursor by then and a retry opens a fresh one, so it is closed before the re-check and one selection needs one connection. TestSimpleDriverBoundedPool pins it with 16 concurrent selectors against a pool of 2, and stalls if the overlapping checkout is restored. The same round adds idx_spendable_amount on (owner_wallet_id, token_type, amount), partial on the spendable-tokens query's own is_deleted/owner/spendable predicates, so its ORDER BY amount reads rows already ordered instead of sorting the wallet - a new index name rather than an extra column on idx_owner_wallet_part, since CREATE INDEX IF NOT EXISTS would not replace an index already deployed under that name. Plus the documented assumption behind bucketedIterator's bucket boundaries: they use string equality on the stored quantity, which identifies equal amounts only because that encoding is canonical while the ORDER BY is numeric, so a non-canonical encoding would degrade the shuffle to a no-op rather than produce a wrong order. IsTerminalStatus is left exported - cmd/tokendiag/cobra/locks/runner.go is a production caller, so it is not test-only. 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. Fourth review round (review 5414277618 on #2410): three Low findings. The TokenLocks DDL was emitted twice, by common.TokenLockStore.GetSchema and common.TokenStore.GetSchema. Both must emit it - the locker owns the table and the token store's notLocked anti-join depends on it - but every statement is CREATE ... IF NOT EXISTS, so whichever store initializes first wins and a future drift between the two copies would resolve silently. They now share one tokenLocksSchema builder, so a change reaches both or neither. stale_candidates_total read zero on exactly the deployments that matter. Postgres is the only BatchLocker, and its claim statement answered with just the tokens it won, which made a stale candidate indistinguishable from a lost race: the drop was booked as LockConflicts, tokensLockedByOthersExist was set although nobody held the token, and the candidate cache was never told it was behind the store. That is now fixed rather than documented, as the previous round's test comment said it should be. LockBatch returns driver.BatchLockOutcome{Won, Stale} and the claim classifies every candidate in the same round trip, so the batch path recovers within the call exactly as the single-token path does. Under skipLocked the spendability predicate is evaluated a second time without the row lock, because a row FOR UPDATE SKIP LOCKED walks past is contended and must not be reported stale; that split is mutation-tested. The single-token Lock path also drops its follow-up isSpendable probe - the claim now reports the state it actually saw, one round trip lighter on that failure path - and a compile-time assertion pins the one production BatchLocker, a capability discovered by type assertion and so able to disappear silently, as benchBatchLocker promptly did. The third finding, that the !hasEnough fast-fail leaves no retry cushion on a lagging read replica, needs no change: before this PR an empty scan with no observed lock conflict returned SelectorInsufficientFunds from that branch unconditionally, and that error exits StubbornSelector's backoff loop outright, so lag failed such a call then too. The check only ever turns a give-up into a retry. Closing the lag window itself is a read-routing question - the candidate scan reads the same replica. The reasoning is recorded in the code and in docs/services/selector.md so it is not re-derived. Tests: the batch stale-candidate test flipped to the recovered behaviour it was written to predict, a new one pinning the degraded no-classification backend, and a real-Postgres classification test covering won/stale/contended in one claim under both batch strategies. 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. Third review round (review 5404492139 on #2410): the simple driver's bounded-pool deadlock is now fixed rather than documented as a known limitation. selectByID held its unspentTokens cursor open across the nested concurrencyCheck query, so every in-flight Select pinned two connections and a pool smaller than the concurrent-selector count deadlocked outright - each connection handed to an open cursor, each goroutine blocked waiting for a second one. The candidate scan is already finished with the cursor by then and a retry opens a fresh one, so it is closed before the re-check and one selection needs one connection. TestSimpleDriverBoundedPool pins it with 16 concurrent selectors against a pool of 2, and stalls if the overlapping checkout is restored. The same round adds idx_spendable_amount on (owner_wallet_id, token_type, amount), partial on the spendable-tokens query's own is_deleted/owner/spendable predicates, so its ORDER BY amount reads rows already ordered instead of sorting the wallet - a new index name rather than an extra column on idx_owner_wallet_part, since CREATE INDEX IF NOT EXISTS would not replace an index already deployed under that name. Plus the documented assumption behind bucketedIterator's bucket boundaries: they use string equality on the stored quantity, which identifies equal amounts only because that encoding is canonical while the ORDER BY is numeric, so a non-canonical encoding would degrade the shuffle to a no-op rather than produce a wrong order. IsTerminalStatus is left exported - cmd/tokendiag/cobra/locks/runner.go is a production caller, so it is not test-only. 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. Fourth review round (review 5414277618 on #2410): three Low findings. The TokenLocks DDL was emitted twice, by common.TokenLockStore.GetSchema and common.TokenStore.GetSchema. Both must emit it - the locker owns the table and the token store's notLocked anti-join depends on it - but every statement is CREATE ... IF NOT EXISTS, so whichever store initializes first wins and a future drift between the two copies would resolve silently. They now share one tokenLocksSchema builder, so a change reaches both or neither. stale_candidates_total read zero on exactly the deployments that matter. Postgres is the only BatchLocker, and its claim statement answered with just the tokens it won, which made a stale candidate indistinguishable from a lost race: the drop was booked as LockConflicts, tokensLockedByOthersExist was set although nobody held the token, and the candidate cache was never told it was behind the store. That is now fixed rather than documented, as the previous round's test comment said it should be. LockBatch returns driver.BatchLockOutcome{Won, Stale} and the claim classifies every candidate in the same round trip, so the batch path recovers within the call exactly as the single-token path does. Under skipLocked the spendability predicate is evaluated a second time without the row lock, because a row FOR UPDATE SKIP LOCKED walks past is contended and must not be reported stale; that split is mutation-tested. The single-token Lock path also drops its follow-up isSpendable probe - the claim now reports the state it actually saw, one round trip lighter on that failure path - and a compile-time assertion pins the one production BatchLocker, a capability discovered by type assertion and so able to disappear silently, as benchBatchLocker promptly did. The third finding, that the !hasEnough fast-fail leaves no retry cushion on a lagging read replica, needs no change: before this PR an empty scan with no observed lock conflict returned SelectorInsufficientFunds from that branch unconditionally, and that error exits StubbornSelector's backoff loop outright, so lag failed such a call then too. The check only ever turns a give-up into a retry. Closing the lag window itself is a read-routing question - the candidate scan reads the same replica. The reasoning is recorded in the code and in docs/services/selector.md so it is not re-derived. Tests: the batch stale-candidate test flipped to the recovered behaviour it was written to predict, a new one pinning the degraded no-classification backend, and a real-Postgres classification test covering won/stale/contended in one claim under both batch strategies. Fifth review round (on #2410): two Low findings, both comment-only. The sufficiency-window lookahead buffer is documented as bounded by sufficiencyWindow rather than by the wallet - nextCandidate dequeues from the buffer before it touches the cache, so a window is assembled out of the buffer first and only sufficiencyWindow-1 entries are ever put back, including in a wallet where every token is individually sufficient. And claimCandidates records that its three spendability-flag placeholders appear twice in the query under skipLocked on purpose: a Postgres $N may be referenced any number of times for a single positional argument, so the reuse must not be mirrored by a second append to args, and keeping them literally the same placeholders is what makes the two CTEs provably the same predicate. 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. Third review round (review 5404492139 on #2410): the simple driver's bounded-pool deadlock is now fixed rather than documented as a known limitation. selectByID held its unspentTokens cursor open across the nested concurrencyCheck query, so every in-flight Select pinned two connections and a pool smaller than the concurrent-selector count deadlocked outright - each connection handed to an open cursor, each goroutine blocked waiting for a second one. The candidate scan is already finished with the cursor by then and a retry opens a fresh one, so it is closed before the re-check and one selection needs one connection. TestSimpleDriverBoundedPool pins it with 16 concurrent selectors against a pool of 2, and stalls if the overlapping checkout is restored. The same round adds idx_spendable_amount on (owner_wallet_id, token_type, amount), partial on the spendable-tokens query's own is_deleted/owner/spendable predicates, so its ORDER BY amount reads rows already ordered instead of sorting the wallet - a new index name rather than an extra column on idx_owner_wallet_part, since CREATE INDEX IF NOT EXISTS would not replace an index already deployed under that name. Plus the documented assumption behind bucketedIterator's bucket boundaries: they use string equality on the stored quantity, which identifies equal amounts only because that encoding is canonical while the ORDER BY is numeric, so a non-canonical encoding would degrade the shuffle to a no-op rather than produce a wrong order. IsTerminalStatus is left exported - cmd/tokendiag/cobra/locks/runner.go is a production caller, so it is not test-only. 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. Fourth review round (review 5414277618 on #2410): three Low findings. The TokenLocks DDL was emitted twice, by common.TokenLockStore.GetSchema and common.TokenStore.GetSchema. Both must emit it - the locker owns the table and the token store's notLocked anti-join depends on it - but every statement is CREATE ... IF NOT EXISTS, so whichever store initializes first wins and a future drift between the two copies would resolve silently. They now share one tokenLocksSchema builder, so a change reaches both or neither. stale_candidates_total read zero on exactly the deployments that matter. Postgres is the only BatchLocker, and its claim statement answered with just the tokens it won, which made a stale candidate indistinguishable from a lost race: the drop was booked as LockConflicts, tokensLockedByOthersExist was set although nobody held the token, and the candidate cache was never told it was behind the store. That is now fixed rather than documented, as the previous round's test comment said it should be. LockBatch returns driver.BatchLockOutcome{Won, Stale} and the claim classifies every candidate in the same round trip, so the batch path recovers within the call exactly as the single-token path does. Under skipLocked the spendability predicate is evaluated a second time without the row lock, because a row FOR UPDATE SKIP LOCKED walks past is contended and must not be reported stale; that split is mutation-tested. The single-token Lock path also drops its follow-up isSpendable probe - the claim now reports the state it actually saw, one round trip lighter on that failure path - and a compile-time assertion pins the one production BatchLocker, a capability discovered by type assertion and so able to disappear silently, as benchBatchLocker promptly did. The third finding, that the !hasEnough fast-fail leaves no retry cushion on a lagging read replica, needs no change: before this PR an empty scan with no observed lock conflict returned SelectorInsufficientFunds from that branch unconditionally, and that error exits StubbornSelector's backoff loop outright, so lag failed such a call then too. The check only ever turns a give-up into a retry. Closing the lag window itself is a read-routing question - the candidate scan reads the same replica. The reasoning is recorded in the code and in docs/services/selector.md so it is not re-derived. Tests: the batch stale-candidate test flipped to the recovered behaviour it was written to predict, a new one pinning the degraded no-classification backend, and a real-Postgres classification test covering won/stale/contended in one claim under both batch strategies. Fifth review round (on #2410): two Low findings, both comment-only. The sufficiency-window lookahead buffer is documented as bounded by sufficiencyWindow rather than by the wallet - nextCandidate dequeues from the buffer before it touches the cache, so a window is assembled out of the buffer first and only sufficiencyWindow-1 entries are ever put back, including in a wallet where every token is individually sufficient. And claimCandidates records that its three spendability-flag placeholders appear twice in the query under skipLocked on purpose: a Postgres $N may be referenced any number of times for a single positional argument, so the reuse must not be mirrored by a second append to args, and keeping them literally the same placeholders is what makes the two CTEs provably the same predicate. Rebased onto main after #2020 landed (perf(storage): index tokens.amount and offer a bounded spendable query), which touched the same spendable-token query. The two are merged rather than either side dropped: - The duplicated idx_spendable_amount DDL - added independently by both - is emitted once. Both copies were textually identical, and keeping both left two %s verbs with no arguments, so GetSchema rendered idx_cleaned_at_%!s(MISSING) and every sqlite schema init failed. - buildSpendableTokensQuery, #2020's shared builder, carries the notLocked anti-join, so a bounded caller cannot see candidates the unbounded iterator hides. - SpendableTokensIteratorBy asks for AmountAscending explicitly, via spendableTokensIteratorByParams. #2020 made AmountUnordered the zero value and let the iterator take it, which is the cheaper plan in general but silently removes the ascending order bucketedIterator and the sufficiency window are built on. - #2020's SQL goldens are updated to the merged shape, and its TestBuildSpendableTokensIteratorByQueryUnchanged - which asserted the iterator emits no ORDER BY and no amount - becomes TestBuildSpendableTokensIteratorByQueryShape, asserting the clauses the selector requires. docs/development/storage.md loses the claim that the selector discards the database's order. 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. Third review round (review 5404492139 on #2410): the simple driver's bounded-pool deadlock is now fixed rather than documented as a known limitation. selectByID held its unspentTokens cursor open across the nested concurrencyCheck query, so every in-flight Select pinned two connections and a pool smaller than the concurrent-selector count deadlocked outright - each connection handed to an open cursor, each goroutine blocked waiting for a second one. The candidate scan is already finished with the cursor by then and a retry opens a fresh one, so it is closed before the re-check and one selection needs one connection. TestSimpleDriverBoundedPool pins it with 16 concurrent selectors against a pool of 2, and stalls if the overlapping checkout is restored. The same round adds idx_spendable_amount on (owner_wallet_id, token_type, amount), partial on the spendable-tokens query's own is_deleted/owner/spendable predicates, so its ORDER BY amount reads rows already ordered instead of sorting the wallet - a new index name rather than an extra column on idx_owner_wallet_part, since CREATE INDEX IF NOT EXISTS would not replace an index already deployed under that name. Plus the documented assumption behind bucketedIterator's bucket boundaries: they use string equality on the stored quantity, which identifies equal amounts only because that encoding is canonical while the ORDER BY is numeric, so a non-canonical encoding would degrade the shuffle to a no-op rather than produce a wrong order. IsTerminalStatus is left exported - cmd/tokendiag/cobra/locks/runner.go is a production caller, so it is not test-only. 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. Fourth review round (review 5414277618 on #2410): three Low findings. The TokenLocks DDL was emitted twice, by common.TokenLockStore.GetSchema and common.TokenStore.GetSchema. Both must emit it - the locker owns the table and the token store's notLocked anti-join depends on it - but every statement is CREATE ... IF NOT EXISTS, so whichever store initializes first wins and a future drift between the two copies would resolve silently. They now share one tokenLocksSchema builder, so a change reaches both or neither. stale_candidates_total read zero on exactly the deployments that matter. Postgres is the only BatchLocker, and its claim statement answered with just the tokens it won, which made a stale candidate indistinguishable from a lost race: the drop was booked as LockConflicts, tokensLockedByOthersExist was set although nobody held the token, and the candidate cache was never told it was behind the store. That is now fixed rather than documented, as the previous round's test comment said it should be. LockBatch returns driver.BatchLockOutcome{Won, Stale} and the claim classifies every candidate in the same round trip, so the batch path recovers within the call exactly as the single-token path does. Under skipLocked the spendability predicate is evaluated a second time without the row lock, because a row FOR UPDATE SKIP LOCKED walks past is contended and must not be reported stale; that split is mutation-tested. The single-token Lock path also drops its follow-up isSpendable probe - the claim now reports the state it actually saw, one round trip lighter on that failure path - and a compile-time assertion pins the one production BatchLocker, a capability discovered by type assertion and so able to disappear silently, as benchBatchLocker promptly did. The third finding, that the !hasEnough fast-fail leaves no retry cushion on a lagging read replica, needs no change: before this PR an empty scan with no observed lock conflict returned SelectorInsufficientFunds from that branch unconditionally, and that error exits StubbornSelector's backoff loop outright, so lag failed such a call then too. The check only ever turns a give-up into a retry. Closing the lag window itself is a read-routing question - the candidate scan reads the same replica. The reasoning is recorded in the code and in docs/services/selector.md so it is not re-derived. Tests: the batch stale-candidate test flipped to the recovered behaviour it was written to predict, a new one pinning the degraded no-classification backend, and a real-Postgres classification test covering won/stale/contended in one claim under both batch strategies. Fifth review round (on #2410): two Low findings, both comment-only. The sufficiency-window lookahead buffer is documented as bounded by sufficiencyWindow rather than by the wallet - nextCandidate dequeues from the buffer before it touches the cache, so a window is assembled out of the buffer first and only sufficiencyWindow-1 entries are ever put back, including in a wallet where every token is individually sufficient. And claimCandidates records that its three spendability-flag placeholders appear twice in the query under skipLocked on purpose: a Postgres $N may be referenced any number of times for a single positional argument, so the reuse must not be mirrored by a second append to args, and keeping them literally the same placeholders is what makes the two CTEs provably the same predicate. Rebased onto main after #2020 landed (perf(storage): index tokens.amount and offer a bounded spendable query), which touched the same spendable-token query. The two are merged rather than either side dropped: - The duplicated idx_spendable_amount DDL - added independently by both - is emitted once. Both copies were textually identical, and keeping both left two %s verbs with no arguments, so GetSchema rendered idx_cleaned_at_%!s(MISSING) and every sqlite schema init failed. - buildSpendableTokensQuery, #2020's shared builder, carries the notLocked anti-join, so a bounded caller cannot see candidates the unbounded iterator hides. - SpendableTokensIteratorBy asks for AmountAscending explicitly, via spendableTokensIteratorByParams. #2020 made AmountUnordered the zero value and let the iterator take it, which is the cheaper plan in general but silently removes the ascending order bucketedIterator and the sufficiency window are built on. - #2020's SQL goldens are updated to the merged shape, and its TestBuildSpendableTokensIteratorByQueryUnchanged - which asserted the iterator emits no ORDER BY and no amount - becomes TestBuildSpendableTokensIteratorByQueryShape, asserting the clauses the selector requires. docs/development/storage.md loses the claim that the selector discards the database's order. Sixth review round (review 5440501678 on #2410): three findings, all about who may release a selection lock. The auditor's finality listener was wired with the real selector-manager provider, although an auditor never acquires selection locks for the transactions it audits - those belong to the node that assembled and spent them. Every transaction it finalized therefore cost an Unlock that could only match zero rows, and a WARN per transaction on a TMS with no usable selector manager. finality.NoSelectorManagerProvider resolves to no selector manager, which releaseLocks already treats as nothing to release, and is wired into auditor.Service.Append and into the evm driver's recovery handler over the audit store, which had the same problem for the same reason; the ttx and transaction-store paths keep the real provider. The sufficiency-window lookahead buffer was shared mutable state on Selector, like cache, but with no memory-safety guard: pending was mutated by slice assignment and append under no lock, while a concurrent Close could write the same field. Every read and write now goes through dequeue, requeue or dropPending, and swapCache and Close clear it, all under s.mu. dequeue makes the closed check first and under that lock, which closes a second gap: a buffered candidate could previously be handed out by an already-closed selector. next() folds into dequeue. A finality status the listener cannot classify says nothing about where the transaction actually stands, so releasing its locks could hand in-flight tokens to a concurrent Select. runOnStatus now reports ErrUnrecognizedStatus, which OnStatus neither retries - calling runOnStatus again with the same arguments can never reclassify the status, so the retry budget and its backoff sleeps bought nothing - nor releases on. Those locks are left to the lease-expiry sweep, exactly as for Busy and Unknown. A recognized terminal status whose local persistence keeps failing still releases, exactly once. applyFinalityLogic needed no change: its default branch already treats an unrecognized status as non-terminal and returns without releasing. Tests: an auditor wiring test asserting a finalized transaction resolves no selector manager, a NoSelectorManagerProvider unit test, a -race test driving concurrent Select and Close over the lookahead buffer, and the two unrecognized-status listener tests inverted to the kept-locks behaviour plus one pinning that the status is not retried. Both new regression tests were verified to fail against the pre-fix code. Signed-off-by: AkramBitar <akram@il.ibm.com>

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

adecaro added bug Something isn't working documentation Improvements or additions to documentation testing db storage token-selector labels Sep 22, 2026

adecaro self-assigned this Sep 22, 2026

adecaro added this pull request to stack #2401 September 22, 2026 12:50

adecaro changed the title ~~test(selector): close testable gaps in lock-contention stack (Phase 7, #2395)~~ test(selector): close lock-contention testable gaps + extend sherdlock benchmarks (Phases 7-8, #2395) Sep 22, 2026

adecaro mentioned this pull request Sep 22, 2026

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

Open

adecaro and others added 2 commits September 22, 2026 17:17

adecaro force-pushed the fix/2395-phase6-lock-strategies branch from 7a61cb8 to 376c892 Compare September 22, 2026 15:21

adecaro force-pushed the fix/2395-phase7-testable-gaps branch from a1cb263 to eeca7bc Compare September 22, 2026 15:21

AkramBitar reviewed Sep 22, 2026

View reviewed changes

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-phase7-testable-gaps branch September 23, 2026 04:59

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

Repository navigation

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

test(selector): close lock-contention testable gaps + extend sherdlock benchmarks (Phases 7-8, #2395)#2403
adecaro wants to merge 2 commits into
fix/2395-phase6-lock-strategiesfrom
fix/2395-phase7-testable-gaps

adecaro 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

Conversation

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

Uh oh!

Summary

Phase 7 — close testable gaps in the lock-contention stack

Phase 8 — extend the selector benchmarks across sherdlock's options, make metrics observable

Numbers, for reference

Test plan

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 22, 2026 •

edited

Loading