fix(selector): anti-join locked tokens and order candidates by amount by adecaro · Pull Request #2399 · LFDT-Panurus/panurus

adecaro · 2026-09-21T13:28:40Z

Summary

Stacked on #2398. Implements Phase 4 of #2395 — the remaining two contributing
mechanisms identified in the stress load-test incident:

4a — anti-join against locked tokens: the spendable-tokens query now excludes
tokens currently held by a lock, via a dialect-independent NOT EXISTS subquery
against TokenLocks (cond.NotExists, added to the DSL). This stops a selector
from starting a race it will lose; the INSERT-based lock acquisition remains
the race-safe backstop, since the anti-join is read-then-act.
- Because the anti-join can hide every remaining candidate from a wallet that
  still has funds (they're just all locked by someone else), an empty scan is
  disambiguated via a new lock-ignoring TokenFetcher.HasAnySpendableTokens
  check before reporting SelectorInsufficientFunds.
- TokenStore.GetSchema now also creates the TokenLocks table itself
  (idempotently, alongside TokenLockStore.GetSchema), since the anti-join makes
  it a hard dependency of the token store, not just of the locker.
4b — size-aware, bucket-shuffled ordering: candidates are ordered ascending by
amount (new ORDER BY), then shuffled only within runs of equal amount
(bucketedIterator.NewPermutation). This answers the incident's "why did a small
request grab a large token instead of a same-size one" question, without
introducing a new hot spot the way a strict smallest-fit rule would.

Only the sherdlock driver is affected; simple is unchanged (unordered, no
anti-join).

Contention baseline (`TestHotTokenContention`)

Before Phase 4 (per-attempt blacklist only, PR #2398):
distinct tokens attempted=300, total lock attempts=4646, total conflicts=4346, conflict rate=0.94, distinct tokens conflicted=296, max single-token conflict share=0.07

After Phase 4 (anti-join + bucketed ordering active):
distinct tokens attempted=300, total lock attempts=3414, total conflicts=3114, conflict rate=0.91, distinct tokens conflicted=212, max single-token conflict share=0.10

Fewer distinct tokens are contended at all (212 vs 296), and total lock attempts/conflicts
drop ~26-28%, consistent with the anti-join keeping already-locked tokens out of the
candidate set entirely.

Docs

Updated docs/services/selector.md (selection algorithm, diagram, driver differences
table) and docs/development/sql-query-dsl.md (cond.NotExists) to reflect the new
sherdlock behaviour.

Test plan

make lint-auto-fix clean
make checks clean
go test ./token/services/selector/... ./token/services/storage/db/sql/... (incl. -race) — all pass
TestHotTokenContention baseline recaptured post anti-join wiring (above)
Integration tests (fabtoken/dlog T1) — not yet run

Part of #2395

AkramBitar

Review of the anti-join + amount-ordering change. I diffed against the PR's actual base (fix/2395-sherdlock-per-attempt-blacklist, since this is stacked on #2398), 14 files.

Bottom line: one blocker, one behaviour change that needs a decision, two things to settle before this reaches Postgres, and no test coverage for any of the new behaviour.

Blocker — HasAnySpendableTokens counts the locks this same Select call just took, so a genuine over-spend now returns SelectorSufficientButLockedFunds after burning every retry (~10s on StubbornSelector) instead of fast-failing with SelectorInsufficientFunds. Wrong sentinel and wrong latency on the most common user error; integration/token/fungible/views/transfer.go:293-295 branches on those sentinels.
Needs a decision — ascending ORDER BY amount makes dust-heavy wallets deterministically exceed MaxInputs: 256, so transfers that previously succeeded will now fail hard.
Before Postgres — the two TokenLocks DDL copies disagree on created_at (TIMESTAMPTZ vs TIMESTAMP) with no init ordering guarantee, and the anti-join silently does nothing if tokendb and tokenlockdb are configured onto different persistences.
Performance / correctness nits — no index covers the new sort on the service's hottest query; bucket boundaries compare the quantity string while the ordering comes from the amount column.

Details inline.

🤖 Generated with Claude Code

AkramBitar · 2026-09-22T11:58:55Z

+				// that every remaining token is currently locked by someone else
+				// and was hidden from us entirely. Disambiguate with a direct,
+				// lock-ignoring existence check before giving up.
+				hasAny, hasAnyErr := s.fetcher.HasAnySpendableTokens(ctx, owner.ID(), tokenType)


HasAnySpendableTokens also ignores our own locks, so genuine insufficient funds no longer fast-fails.

The check is lock-ignoring by design, but that includes the locks this very Select call just acquired — tokens we locked are still spendable = true, is_deleted = false.

Scenario: wallet holds two tokens worth 3 and 2, caller requests 10. Pass 1 locks both, sum = 5 < 10, the cache exhausts, tokensLockedByOthersExist == false. HasAnySpendableTokens sees our own two rows, returns true, and the SelectorInsufficientFunds return below is skipped. The refetch then returns nothing (the new notLocked anti-join hides the tokens we hold), so the loop burns all maxImmediateRetries — 6 extra spendable queries plus 6 extra HasAnySpendableTokens queries — and returns token.SelectorSufficientButLockedFunds.

Two consequences:

For the plain Selector (backoff < 0) that is a different sentinel than before this PR, and integration/token/fungible/views/transfer.go:293-295 branches on exactly those two sentinels.

For StubbornSelector it additionally costs numRetries+1 = 4 backoff rounds of up to retryInterval = 5s each (~10s p50) before finally reporting insufficient funds, with the misleading message "aborted too many times and no other process unlocked or added tokens".

So the most common user error — asking to spend more than you have — goes from milliseconds to seconds, and reports the wrong reason. Suggest gating the check on selected.Empty(), or excluding locks whose consumer_tx_id is this consumer tx.

AkramBitar · 2026-09-22T11:58:55Z

+			}, nil),
+			notLocked(tokenTable, tokenLocksTable),
+		)).
+		OrderBy(q.Asc(common3.FieldName("amount"))).


Ascending ORDER BY amount can deterministically exceed limits.MaxInputs.

The docs note input count may increase, but not that it can now hard-fail. token/driver/limits.go:60 sets MaxInputs: 256, enforced in token/core/fabtoken/v1/actions/transfer.go:214 and token/core/zkatdlog/nogh/v1/issue/action.go:237 with ErrTooManyInputs.

Scenario: a wallet holding 1000 tokens of 1 unit plus 5 tokens of 1000 units transfers 1000 units. Ascending order picks 1000 one-unit tokens and the action is rejected. Under the previous uniform shuffle the expected draw was ~10.9 units, so ~92 inputs — comfortably under the cap.

That turns a statistically-succeeding transfer into a deterministically-failing one for dust-heavy wallets. Worth capping the ascending preference, e.g. skipping to a larger bucket once the selected count approaches MaxInputs.

AkramBitar · 2026-09-22T11:58:55Z

+			tx_id TEXT NOT NULL,
+			idx INT NOT NULL,
+			consumer_tx_id TEXT NOT NULL,
+			created_at TIMESTAMPTZ NOT NULL,


This DDL disagrees with TokenLockStore.GetSchema on created_at, so the effective column type depends on init order.

Here: created_at TIMESTAMPTZ NOT NULL. In token/services/storage/db/sql/common/tokenlock.go:97: created_at TIMESTAMP NOT NULL. Both are CREATE TABLE IF NOT EXISTS, and Driver.TokenLock / Driver.Token are independent lazy.Providers with no ordering guarantee — the sibling newWalletStoreProvider comment at sqlite/driver.go:120 calls exactly this out for the Identity/Wallet FK. Whichever store initializes first wins.

On Postgres this changes lock expiry: cond.OlderThan renders created_at < NOW() - INTERVAL 'N seconds' (postgres/conditions.go:24), and TIMESTAMP is cast to timestamptz using the session TimeZone while the values were written as time.Now().UTC() — so on a non-UTC session stale locks expire off by the zone offset, whereas with TIMESTAMPTZ they expire correctly.

Also worth noting: dbtest/tokenlock.go:28 creates the token store first, so CI now exercises TIMESTAMPTZ while a deployment that touches the lock store first gets TIMESTAMP — the tests no longer cover production's schema. Best to make the two statements byte-identical, or keep the DDL in one place.

AkramBitar · 2026-09-22T11:58:55Z

+// This is deliberately not folded into HasTokenDetails: balance and audit
+// queries need to see locked tokens too, only the spendable-tokens query
+// used by the selector should exclude them.
+func notLocked(tokenTable, tokenLocksTable common3.Table) cond.Condition {


The anti-join silently no-ops when tokendb and tokenlockdb resolve to different persistences.

notLocked joins db.table.TokenLocks inside the token store's own database and table-name space, but the two stores are configured through independent keys — tokendb.persistence (token/services/storage/tokendb/store.go:28) and tokenlockdb.persistence (token/services/storage/tokenlockdb/store.go:18) — each resolving its own fscSqlite.Config/Postgres DSN and its own TablePrefix via GetTableNamesWithOverrides.

If an operator points them at different databases, or the same database with different prefixes, or one at memory and one at postgres, then TokenStore.GetSchema happily creates a second, permanently empty TokenLocks table in the token DB, the NOT EXISTS matches nothing, and the whole mitigation disappears with no error, warning or log line — while HasAnySpendableTokens still fires on every exhausted scan. This new hard dependency needs either a startup validation that the two share a persistence, or an explicit documented constraint.

AkramBitar · 2026-09-22T11:58:55Z

@@ -391,18 +397,47 @@ func (it *dedupedTokenRowsIterator) Next() (*token.UnspentToken, error) {
 // can compare the dynamic path against a prepared-once path using identical


No index supports the new sort on the hottest query in the service.

buildSpendableTokensIteratorByQuery is what the cached fetcher runs across all wallets and types (fetcher.go:376, SpendableTokensIteratorBy(ctx, "", "")) at defaultCacheFreshnessInterval = 1 * time.Second / every 5 queries. The tokens schema (tokens.go:1421-1426) has idx_owner_wallet_part (owner_wallet_id, token_type) WHERE is_deleted = false AND owner = true but nothing covering amount, so Postgres must add a full sort of the matched set on every refresh and every lazy fetch, on top of the new correlated NOT EXISTS.

Adding amount to the partial index would avoid it — or drop the global ORDER BY and sort the grouped slices in Go, since groupTokensByKey already materializes them.

AkramBitar · 2026-09-22T11:58:55Z

+
+	for start := 0; start < len(shuffled); {
+		end := start + 1
+		for end < len(shuffled) && shuffled[end].Quantity == shuffled[start].Quantity {


Buckets are detected on the Quantity string while the ordering comes from the amount column.

This compares the quantity TEXT column, but the run structure it assumes was produced by ORDER BY amount on a separate NUMERIC(78,0) column. The two are written independently — token/services/tokens/storage.go:208-210: Quantity: tta.Tok.Quantity verbatim from the action, Amount: q.ToBigInt().Uint64() derived.

Any non-canonical spelling difference between two equal-valued tokens (e.g. 0x0a vs 0xa, or a decimal vs hex encoding from a different driver/peer version) splits one amount bucket into singleton buckets that are never shuffled against each other, restoring exactly the deterministic hot-spot the bucketing exists to prevent — and silently, since the ordering still looks correct. Comparing the numeric amount (or carrying it on UnspentTokenInWallet) would make the bucket boundary agree with the ORDER BY by construction.

AkramBitar · 2026-09-22T11:58:55Z

+// refetch branch) signals exhaustion with a nil element and nil error, not
+// io.EOF — returning io.EOF here made every lazy-fetch refetch cycle look
+// like a hard failure instead of "cache exhausted, fetch more" (#2395).
+func (b *bucketedIterator) Next() (*token2.UnspentTokenInWallet, error) {


Nit: this Godoc justifies itself with a bug that never existed on this path.

The comment says returning io.EOF "made every lazy-fetch refetch cycle look like a hard failure instead of 'cache exhausted, fetch more' (#2395)". The code being replaced was collections.NewPermutatedIterator → FSC iterators.Permutate → iterators.Slice, whose Next returns (zero, nil) — i.e. (nil, nil) for a pointer element — at exhaustion (fabric-smart-client@v0.16.0/.../iterators/slice.go:43-46).

The (nil, nil) contract is right; only the stated rationale is wrong, and it will mislead the next reader into thinking a regression was fixed here.

AkramBitar · 2026-09-22T11:58:55Z

 	return args.Get(0).(driver.SpendableTokensIterator), args.Error(1)
 }

+func (m *mockTokenDB) HasAnySpendableTokens(ctx context.Context, walletID string, typ token2.Type) (bool, error) {


None of the new behaviour has a test.

The only test-file changes in this PR are mock/stub methods to satisfy the widened interfaces. Nothing asserts that:

the emitted SQL contains the NOT EXISTS clause or the ORDER BY amount — TestSpendableTokensIteratorByPreparedReuse only compares the dynamic and prepared paths to each other, so it would pass identically if either clause were dropped;

a locked token is actually excluded end-to-end;

bucketedIterator.NewPermutation preserves cross-bucket ordering while permuting within a bucket;

HasAnySpendableTokens ignores locks.

Per AGENTS.md's testing conventions each of these is a cheap table-driven or SQL-text assertion, and the HasAnySpendableTokens / MaxInputs / Quantity-bucketing issues flagged elsewhere in this review are all the kind of regression such a test would have caught.

Adds the two remaining #2395 contributing mechanisms: the spendable-tokens query now excludes already-locked tokens via a NOT EXISTS anti-join against TokenLocks (cond.NotExists), and orders remaining candidates ascending by amount, shuffling only within same-amount buckets so contention still spreads across equally-good candidates instead of concentrating on one. Because the anti-join can hide every remaining candidate from a wallet that still has funds (they are just all locked by someone else), selectInternal disambiguates an empty scan with a new lock-ignoring HasAnySpendableTokens check before reporting insufficient funds. TokenStore.GetSchema now also creates the TokenLocks table itself, since the anti-join makes it a hard dependency of the token store, not just of the locker. Part of #2395 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>

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

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

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 mentioned this pull request Sep 21, 2026

fix(selector): release token locks on transaction settlement #2400

Closed

4 of 5 tasks

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

adecaro mentioned this pull request Sep 22, 2026

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

Merged

8 tasks done

AkramBitar reviewed Sep 22, 2026

View reviewed changes

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-per-attempt-blacklist branch from b5d9524 to e691fb5 Compare September 22, 2026 15:20

adecaro force-pushed the fix/2395-sherdlock-antijoin-ordering branch from 655b435 to 0781e82 Compare September 22, 2026 15:20

adecaro mentioned this pull request Sep 23, 2026

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

Open

5 tasks done

adecaro closed this Sep 23, 2026

adecaro deleted the fix/2395-sherdlock-antijoin-ordering branch September 23, 2026 04:58

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

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

fix(selector): anti-join locked tokens and order candidates by amount#2399
adecaro wants to merge 1 commit into
fix/2395-sherdlock-per-attempt-blacklistfrom
fix/2395-sherdlock-antijoin-ordering

adecaro commented Sep 21, 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!

AkramBitar Sep 22, 2026

Uh oh!

AkramBitar Sep 22, 2026

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

		@@ -391,18 +397,47 @@ func (it dedupedTokenRowsIterator) Next() (token.UnspentToken, error) {
		// can compare the dynamic path against a prepared-once path using identical

Conversation

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

Uh oh!

Summary

Contention baseline (TestHotTokenContention)

Docs

Test plan

Uh oh!

AkramBitar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adecaro commented Sep 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adecaro commented Sep 21, 2026 •

edited

Loading

Contention baseline (`TestHotTokenContention`)