sherdlock: add lock-contention diagnostics and a reproducible baseline test (Phases 1-2 of #2395) by adecaro · Pull Request #2397 · LFDT-Panurus/panurus

adecaro · 2026-09-21T10:52:18Z

Summary

Part of #2395 — a stress load test showed 0.47% of tokens absorbing 95.2% of
zkat_token_locks_pkey violations under the sherdlock token selector. This
PR covers Phases 1-2 of the fix plan: diagnostics and a reproducible
contention baseline, no behavioral change to selection itself, so later
phases (blacklist, anti-join, lock release on settlement) have a real
yardstick to measure against.

Baseline-fidelity note: TestHotTokenContention (Phase 2) reproduces
contention under load — many concurrent selectors racing on a
small-UTXO wallet — not the CERT incident's specific static-hot-token
Pareto shape (5 static token rows absorbing 85% of collisions). Because
deleteTokensAndStoreChange mints a fresh token ID each time the hot
token is spent, this baseline's contention is spread across ~300 rotating
token IDs with no single ID dominating — see the Phase 2 section below for
the recorded numbers. Treat it as a load-shape regression guard, not a
byte-for-byte CERT reproduction.

Phase 1 — Diagnostics

Read API for held locks. driver.TokenLockStore gains ListLocks.
Fixed a latent cross-dialect bug while adding it: modernc.org/sqlite
only auto-converts DATE/DATETIME/TIMESTAMP columns to time.Time on
Scan, not TIMESTAMPTZ (which the shared schema deliberately uses for
timezone-consistent comparison against Postgres's NOW()). Added a
scannableTime custom sql.Scanner that handles both dialects' wire
formats. Also adds cond.NotExists to the query DSL, a prerequisite
building block for the Phase 4 anti-join.
tokendiag locks CLI. New cmd/tokendiag module (mirrors
cmd/skicleanup's pattern) with a locks subcommand that connects
directly to an existing Panurus database and reports every held lock with
its age and consumer tx status, flagging locks whose consumer is already
terminal (Confirmed/Deleted/Orphan) as leaked — this is
mechanism 4 from the issue: nothing releases a lock on settlement, so it
sits until the next lease sweep.
Contention metrics + structured logging. sherdlock/metrics.go gains
LockConflicts (deliberately unlabeled by token/wallet id — unbounded
cardinality) and DistinctTokensAttempted (per Select() call), to tell
apart "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.

Phase 2 — Reproducible contention baseline

testutils.TestHotTokenContention. A workload shaped like the
reported incident: a wallet with a few small tokens plus one large,
rotating hot token, and far more concurrent requests than tokens across
multiple replicas.
countingLocker decorator + startManagersWithLockCounters in
sherdlock/manager_test.go, wrapping the real Postgres-backed Locker to
record per-token lock attempts and conflicts (driver.ErrTokenAlreadyLocked)
without duplicating the manager wiring.
Baseline recorded (3 replicas × 100 requests, real Postgres via
testcontainers): 300 distinct tokens attempted, ~7500 total lock attempts,
~96% conflict rate, no single token ID absorbing a large share of
conflicts by itself — because deleteTokensAndStoreChange mints a fresh
token ID each time the hot token is spent, so it is the lineage that
stays hot, not one static ID. The only hard assertion is the functional
invariant that must hold regardless of how contention is distributed:
total demand exactly equals total wallet balance, so no error can be a
genuine insufficient-funds — any error observed would be spurious,
contention-induced. This baseline is the yardstick Phases 3-5 should
improve on.

Follow-up: diagnostics tooling and CI fixes

Address feedback surfaced during broader review of the #2395 stack:

tokendiag locks couldn't resolve table names against a real
deployment. GetTableNamesWithConfig needs params (network/channel/
namespace) for shared-schema table naming, but Config had no field to
carry them and stores.go called it with none. Added
TableNameParams []string to Config, threaded it through, documented
it in cmd/tokendiag/README.md / docs/development/tokendiag.md, and
added stores_test.go proving params are load-bearing (lookup breaks
without them, works with them).
token/services/metricsdoc golden file was stale — missing rows for
the two new sherdlock metrics from Phase 1. Regenerated
testdata/metrics.golden; this alone also resolved the
unit-tests-race/unit-tests-regression CI failures, which were both
failing solely on TestMetricsReference.
Replaced all 11 fmt.Errorf call sites in
cmd/tokendiag/cobra/locks/{stores,config,runner}.go with
github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors per
AGENTS.md convention.
itest (fabricx-dlog-t3) CI failure root-caused as an infra flake
(Docker Hub connection reset pulling hashicorp/vault during
make testing-docker-images), unrelated to this PR's diff.

Test plan

go build ./... and cd cmd/tokendiag && go build ./...
make lint-auto-fix — 0 issues across every module
make checks — passes (the one flagged vuln, GO-2024-3218 in
go-libp2p-kad-dht, is pre-existing and allow-listed)
go test ./token/services/storage/db/... — ListLocks covered on
both sqlite and postgres via the shared dbtest suite
go test ./token/services/selector/... and go test -race ./token/services/selector/sherdlock/... — including the new
TestHotTokenContention against real Postgres
cd cmd/tokendiag && go test ./... — new stores_test.go covering
TableNameParams
go test ./token/services/metricsdoc/... — TestMetricsReference
passes against the regenerated golden file
Docs updated: docs/README.md tools table, docs/development/
index, new docs/development/tokendiag.md, and the sherdlock metrics
table in docs/development/metrics.md

Part of #2395

🤖 Generated with Claude Code

Effi-S

Some low priority notes:

Run docstring over-claims a "hot-token ranking" section cmd/tokendiag/cobra/locks/runner.go:50-57

The docstring lists four report sections, including "a hot-token ranking, oldest lock first" as distinct from "every held lock … with its age". The implementation prints only one oldest-first list plus a summary; there is no separate ranking/aggregation. Either trim the docstring to match, or clarify that the oldest-first list is the ranking.

Stores.db field is dead state (cmd/tokendiag/cobra/locks/stores.go:24). NewStores stores the raw *sql.DB in Stores.db, but Close() only calls s.TokenLock.Close() (which closes the same handle via common2.Close), and nothing else ever reads db. Remove the field, as written it suggests to a reader that Close is incomplete.
Duplicated attempted.Add(t.Id) (selector.go:214 and :228). Every token reaching TryLock is "attempted" regardless of outcome, so a single attempted.Add(t.Id) just after obtaining t (before the TryLock branch) is simpler and can't be forgotten if a third outcome branch is added later.
Nil-Status (LEFT JOIN) path is documented but untested. LockRecord.Status can be nil when a lock's consumer has no requests row; TestListLocks only covers Pending/Confirmed. A one-line case with an orphaned lock would lock in the documented behavior and the statusName→"unknown" / isTerminal(nil)==false handling.
DistinctTokensAttempted skips rate-limited attempts. The token.SelectorRateLimited branch returns before attempted.Add(t.Id) (selector.go:212), so a token denied by rate-limiting isn't counted as "attempted." Defensible (it wasn't really attempted), just worth a one-line comment so the histogram's semantics are unambiguous.

AkramBitar · 2026-09-22T09:32:21Z

Review

Summary

Good diagnostics foundation — the ListLocks API, tokendiag CLI, contention metrics, and baseline test are exactly the right building blocks for the later fix phases. Two blockers need addressing before merge.

Blocker 1 — `ListLocks` uses INNER JOIN (`tokenlock.go`)

The join against requests on consumer_tx_id = tx_id drops any lock whose consumer transaction row does not exist yet — which includes every in-flight transfer and every leaked lock. The tool reports 0 locks against a full table. Needs a LEFT JOIN so that Status == nil is actually reachable as documented.

Blocker 2 — `tokendiag` resolves table names without `TableNameParams` (`stores.go`)

GetTableNamesWithConfig produces names like fsc_tkn_locks_<prefix>_<params>. Without the params component the CLI queries a non-existent table and errors out on any real node deployment.

Bug — `DistinctTokensAttempted` is recorded per retry, not per `Select()` call

The attempted set lives inside selectInternal, which is called once per retry iteration. The histogram records fan-out per retry, not per user-facing Select() call, so it does not measure what it claims. Fix: move the set to the Select() caller.

Nit — `fmt.Errorf` in `cmd/tokendiag/cobra/locks/`

config.go, runner.go, and stores.go use fmt.Errorf to wrap errors. Project rule requires github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors instead (errors.Wrapf, errors.Errorf).

AkramBitar

@adecaro
See my comments.
Thanks a lot,
Akram

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>

AkramBitar

What we added on top of this PR:

cmd/tokendiag/cobra/locks/config.go — CORE_* overrides were silently ignored. Unmarshal walks AllKeys(), which never contains env-only keys, so CORE_DATASOURCE tripped the empty-dataSource check and CORE_TABLEPREFIX / CORE_TABLENAMEPARAMS resolved the wrong tables. Each overridable key is now bound up front.
token/services/storage/db/sql/common/tokenlock.go — ListLocks returned a truncated slice with a nil error on a mid-scan failure, because the shared rowIterator never consults rows.Err(). Now checked.
token/services/selector/sherdlock/selector.go — DistinctTokensAttempted was observed once per backoff round instead of once per Select(), inflating _count and understating fan-out. The accumulator now lives in the caller and is unioned across rounds.

Tests: TestLoadConfigEnvOverrides, TestDistinctTokensAttemptedObservedOncePerSelect.

AkramBitar · 2026-09-24T12:41:27Z

@Effi-S

Could you please have addition review round on latest fixes improvements?

Regards,
Akram

By mistake

Effi-S

LGTM

Phases 1-2 of #2395: make sherdlock's lock contention observable, and give the behaviour a reproducible baseline before changing the selector itself. - Metrics: a lock_conflicts_total counter and a distinct_tokens_attempted histogram. The histogram is observed exactly once per Select() call, with the attempted-token set accumulated in the caller and unioned across every backoff round, so a StubbornSelector retry does not emit one sample per round; a call that never attempted a lock records nothing rather than a zero sample below the first bucket. - A reproducible hot-token contention baseline test, plus shared selector test cases. - driver.TokenLockStore.ListLocks and driver.LockRecord: read every held lock joined with the status of its consuming transaction. The SQL implementation checks rows.Err() after draining the iterator, because the shared rowIterator does not, and a mid-scan failure would otherwise return a truncated list with a nil error. - cond.Exists, for correlated-subquery conditions. - tokendiag: a new CGO-free CLI (cmd/tokendiag) that reports the currently held token locks with their age and consumer status, flags locks whose consumer has already reached a terminal status, and prints a script-friendly summary. Configuration is a YAML file; every scalar key can be overridden by a CORE_* environment variable, each bound up front so that viper's Unmarshal, which walks AllKeys(), actually visits keys supplied only through the environment. - Documentation: docs/development/tokendiag.md, cmd/tokendiag/README.md and the metrics reference. Signed-off-by: Angelo De Caro <adc@zurich.ibm.com> Co-authored-by: AkramBitar <akram@il.ibm.com> Signed-off-by: AkramBitar <akram@il.ibm.com>

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 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 requested review from AkramBitar, Effi-S and alexandrosfilios September 21, 2026 11:11

adecaro changed the title ~~sherdlock: add lock-contention diagnostics (Phase 1 of #2395)~~ sherdlock: add lock-contention diagnostics and a reproducible baseline test (Phases 1-2 of #2395) Sep 21, 2026

adecaro mentioned this pull request Sep 21, 2026

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

Closed

4 tasks done

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

Effi-S previously approved these changes Sep 22, 2026

View reviewed changes

Comment thread token/services/selector/sherdlock/selector.go Outdated

Comment thread token/services/selector/sherdlock/selector.go Outdated

AkramBitar requested changes Sep 22, 2026

View reviewed changes

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-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 force-pushed the fix/2395-sherdlock-lock-contention-diagnostics branch from 58cde2e to d26ce8e Compare September 22, 2026 15:25

adecaro marked this pull request as draft September 23, 2026 05:03

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

adecaro force-pushed the fix/2395-sherdlock-lock-contention-diagnostics branch from d26ce8e to 71ee736 Compare September 23, 2026 05:09

adecaro added this pull request to stack #2411 September 23, 2026 05:09

AkramBitar self-assigned this Sep 24, 2026

AkramBitar previously approved these changes Sep 24, 2026

View reviewed changes

AkramBitar marked this pull request as ready for review September 24, 2026 12:22

AkramBitar force-pushed the fix/2395-sherdlock-lock-contention-diagnostics branch from 56b79e3 to 3481f2e Compare September 24, 2026 12:37

AkramBitar requested review from AkramBitar and Effi-S September 24, 2026 12:40

AkramBitar force-pushed the fix/2395-sherdlock-lock-contention-diagnostics branch from 3481f2e to c88ae2f Compare September 24, 2026 12:59

Effi-S mentioned this pull request Sep 24, 2026

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

Open

Effi-S approved these changes Sep 27, 2026

View reviewed changes

Effi-S mentioned this pull request Sep 27, 2026

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

Open

Effi-S force-pushed the fix/2395-sherdlock-lock-contention-diagnostics branch from c88ae2f to 2a41ac0 Compare September 28, 2026 08:23

AkramBitar force-pushed the fix/2395-sherdlock-lock-contention-diagnostics branch from 2a41ac0 to 15512c2 Compare September 29, 2026 12:01

AkramBitar force-pushed the fix/2395-sherdlock-lock-contention-diagnostics branch from 15512c2 to babcb4b Compare September 29, 2026 12:05

AkramBitar merged commit 019d794 into main Sep 29, 2026
214 checks passed

AkramBitar deleted the fix/2395-sherdlock-lock-contention-diagnostics branch September 29, 2026 12:41

Navigation Menu

Repository navigation

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

sherdlock: add lock-contention diagnostics and a reproducible baseline test (Phases 1-2 of #2395)#2397
AkramBitar merged 1 commit into
mainfrom
fix/2395-sherdlock-lock-contention-diagnostics

adecaro commented Sep 21, 2026 •

edited

Loading

Uh oh!

Effi-S left a comment

Uh oh!

Uh oh!

Uh oh!

AkramBitar commented Sep 22, 2026

Uh oh!

AkramBitar left a comment

Uh oh!

AkramBitar left a comment •

edited

Loading

Uh oh!

AkramBitar commented Sep 24, 2026

Uh oh!

Effi-S left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

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

Uh oh!

Summary

Phase 1 — Diagnostics

Phase 2 — Reproducible contention baseline

Follow-up: diagnostics tooling and CI fixes

Test plan

Uh oh!

Effi-S left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AkramBitar commented Sep 22, 2026

Review

Summary

Blocker 1 — ListLocks uses INNER JOIN (tokenlock.go)

Blocker 2 — tokendiag resolves table names without TableNameParams (stores.go)

Bug — DistinctTokensAttempted is recorded per retry, not per Select() call

Nit — fmt.Errorf in cmd/tokendiag/cobra/locks/

Uh oh!

AkramBitar left a comment

Choose a reason for hiding this comment

Uh oh!

AkramBitar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkramBitar commented Sep 24, 2026

Uh oh!

Effi-S left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adecaro commented Sep 21, 2026 •

edited

Loading

Blocker 1 — `ListLocks` uses INNER JOIN (`tokenlock.go`)

Blocker 2 — `tokendiag` resolves table names without `TableNameParams` (`stores.go`)

Bug — `DistinctTokensAttempted` is recorded per retry, not per `Select()` call

Nit — `fmt.Errorf` in `cmd/tokendiag/cobra/locks/`

AkramBitar left a comment •

edited

Loading