fix(selector): release token locks on transaction settlement by adecaro · Pull Request #2400 · LFDT-Panurus/panurus

adecaro · 2026-09-21T14:46:00Z

Summary

Stacked on #2399. Implements Phase 5 (final phase) of #2395 — the fourth
contributing mechanism identified in the stress load-test incident: no
production code path released a transaction's selection locks on success.

Transaction.Release (the only production Unlock caller) was registered
solely against context.OnError — view.Context exposes no success hook. So
a settled transaction's locks sat until the sherdlock lease-expiry sweep
(several minutes, by config), during which its tokens stayed invisible to the
Phase 4 anti-join and kept colliding.

Both places that already compute terminal transaction status now release
locks once that status is known: finality.Listener.runOnStatus (the live
finality subscription) and TTXRecoveryHandler.applyFinalityLogic
(recovery on restart) — for both Confirmed and Deleted, since a
failed transaction will never spend the tokens it selected either.
A new finality.SelectorManagerProvider resolves the token.SelectorManager
bound to a fixed TMS, re-resolving it on every call rather than caching it —
mirroring the existing TokenRequestHasher.ProcessTokenRequest pattern.
Release is best-effort: a failed Unlock is logged and does not fail the
settlement/recovery path (mirrors Transaction.Release's existing style);
the lease-expiry sweep remains the backstop for Orphan consumers (which the
finality path never observes) and for any release call that failed.
Wired at all three production construction sites: ttx.Service.Append,
auditor.Service.Append (a harmless no-op unlock for the auditor role, which
never itself locks tokens), and the Fabric recovery-manager wiring.

Why the contention benchmark shows no change here

TestHotTokenContention (the harness used to baseline Phases 3-4) is
structurally blind to this mechanism: it calls Select and then immediately
deletes/spends the tokens synchronously against the store, never routing
through the TTX finality settlement path where this PR's Unlock hook lives.
Re-running it before/after this change on the same environment gives identical
numbers:

distinct tokens attempted=300, total lock attempts=3353, total conflicts=3053,
conflict rate=0.91, distinct tokens conflicted=202, max single-token conflict share=0.10

This is expected, not a regression — mechanism 4 is a settlement-timing bug
(locks outliving a settled transaction by minutes), not a selection-time
contention bug, so it needs a harness that actually waits on finality to
observe. The correct verification for this PR is at the unit level: new tests
assert Unlock fires exactly once per transaction, for both Confirmed and
Deleted, and that a failing Unlock does not fail settlement/recovery
(TestOnStatus_ReleasesLocksAfterTerminalStatus,
TestTTXRecoveryHandler_Recover_ReleasesLocksOnConfirmed,
TestTTXRecoveryHandler_Recover_ReleasesLocksOnDeleted,
TestTTXRecoveryHandler_Recover_LockReleaseErrorDoesNotFailRecovery).

Docs

Updated docs/services/selector.md — new "Release on settlement" section, and
reframed "Lease expiry" as the backstop it now is rather than the sole release
mechanism.

Test plan

make lint-auto-fix clean
make checks clean
go test ./token/services/ttx/... ./token/services/auditor/... ./token/services/network/fabric/... (incl. -race) — all pass
New unit regression tests for lock release on Confirmed/Deleted, and for release-error not failing settlement
Integration tests (fabtoken/dlog T1) — not yet run

Fixes #2395

AkramBitar

Logic is correct and tests/vet are clean. Two issues undercut the fix, plus the self-flagged [ ] integration tests.

AkramBitar · 2026-09-22T11:08:20Z

 	} else {
 		t.metrics.DeletedTransactions.Add(1)
 	}
+	releaseLocks(ctx, t.logger, t.selectorManagerProvider, txID)


Release runs after Commit, which ends in NotifyStatus. The woken client can return from Finality and start its next Select while these lock rows still exist, so the Phase-4 anti-join keeps hiding the tokens — the contention this PR removes.

Release before NotifyStatus, or defer it at the top of runOnStatus.

AkramBitar · 2026-09-22T11:08:20Z

+	if sm == nil {
+		return
+	}
+	if err := sm.Unlock(ctx, txID); err != nil {


ctx is the caller's cancellable context. recovery.Manager.callHandler abandons this goroutine when TransactionTimeout fires, but tx.Commit() takes no ctx and NotifyStatus uses WithoutCancel — so execution reaches here with a canceled ctx, the DELETE fails instantly, and the lock waits out the full leaseExpiry.

Use context.WithoutCancel(ctx), as Transaction.Release already does ("we need to unlock even if t.Context is canceled").

A transaction's selection locks previously lingered until the sherdlock lease-expiry sweep (several minutes by default), regardless of whether the transaction had already reached a terminal status. During that window the already-spent-for tokens stayed locked and invisible to concurrent selectors, making them collide on the same hot tokens (mechanism 4 of #2395). Release the locks as soon as a transaction's status is known to be terminal, in the two places that already compute it: the live finality listener (Listener.runOnStatus) and the recovery path (TTXRecoveryHandler.applyFinalityLogic). Both Confirmed and Deleted transactions release, since a failed transaction will never spend its selected tokens either. Release is best-effort and never fails the settlement/recovery path; the lease-expiry sweep remains as a backstop for Orphan consumers and any release call that failed. A new finality.SelectorManagerProvider resolves the token.SelectorManager for a fixed TMS, re-resolving it on every call rather than caching it, mirroring TokenRequestHasher's existing pattern. Fixes #2395 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>

…ase 6, #2395) Stacked on #2400. Attacks the lock-acquisition mechanism itself, on top of the 4-PR correctness stack: every replica still reads the same anti-join snapshot, picks the same smallest candidate, and raced a plain INSERT, surfacing a lost race as a server-side unique-constraint violation. - token.storage.db.lockStrategy selects insert (default, unchanged) / onConflict / skipLocked on the Postgres TokenLockStore. sqlite reads and ignores the key; simple never reaches this config path, so both are untouched by construction. - onConflict and skipLocked replace the plain INSERT with INSERT ... ON CONFLICT DO NOTHING RETURNING: a lost race is a clean zero-row result, not a server error. - skipLocked additionally implements BatchLocker.LockBatch: a covering window of candidates is claimed in one statement against the Tokens rows under FOR UPDATE OF <tokens> SKIP LOCKED, so a claimant walks past a row a concurrent claimant is already mid-claim on. selectInternal type-asserts for BatchLocker and claims the whole window in one round trip when available, falling back to the one-at-a-time Lock path otherwise. - HasEnoughSpendableTokens (common/tokens.go) lets the empty-anti-join fallback fail immediately on a genuinely unpayable wallet instead of burning the full backoff budget first. - Round-trip / unique-violation instrumentation (RoundTrips/UniqueViolations on TokenLockStore) makes the actual effect measurable directly, since the literal lock-conflict rate does not move across strategies: FOR UPDATE SKIP LOCKED only helps against a rival mid-claim at the same instant, not against an already-committed lock, the dominant conflict mode under load. Verified against real Postgres: insert produces real unique-constraint violations on the single-token Lock path (thousands in one run); onConflict/skipLocked produce zero. Round-trip savings come from batching the claim into one statement per covering window, uniform across strategies, not from strategy choice. New tests: TestTokenLockStore_LockBatch_SkipLocked_SkipsRowLockedByConcurrentTx proves the SKIP LOCKED mechanism directly against real Postgres (skips a row a concurrent tx holds; a plain FOR UPDATE on the same row genuinely blocks). TestHotTokenContention_SingleTokenLockPath forces the single-token Lock path via a singleTokenOnlyLocker adapter to give the unique-violation counter its first genuinely differentiated result. TestHotTokenContention and TestHotTokenContentionWideWindow now run across all three strategies. Docs: docs/services/selector.md gets a new "Lock-acquisition strategies" section, scoping the unique-violation-elimination benefit to the single-token Lock path (LockBatch already avoids the server error under every strategy once a store implements BatchLocker) and the round-trip saving to batching itself. Fixes #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:35Z

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>

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 added this pull request to stack #2401 September 21, 2026 14:52

This was referenced Sep 22, 2026

feat(selector): configurable Postgres lock-acquisition strategies (Phase 6, #2395) #2402

Closed

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

Merged

AkramBitar reviewed Sep 22, 2026

View reviewed changes

AkramBitar requested changes 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-antijoin-ordering branch from 655b435 to 0781e82 Compare September 22, 2026 15:20

adecaro force-pushed the fix/2395-release-locks-on-settlement branch from 1892c06 to d3ef574 Compare September 22, 2026 15:21

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-release-locks-on-settlement 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): release token locks on transaction settlement - #2400

fix(selector): release token locks on transaction settlement#2400
adecaro wants to merge 1 commit into
fix/2395-sherdlock-antijoin-orderingfrom
fix/2395-release-locks-on-settlement

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!

adecaro commented Sep 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

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

Uh oh!

Summary

Why the contention benchmark shows no change here

Docs

Test plan

Uh oh!

AkramBitar left a comment

Choose a reason for hiding this comment

Uh oh!

AkramBitar Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

AkramBitar Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

adecaro commented Sep 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adecaro commented Sep 21, 2026 •

edited

Loading