Conversation
adecaro
added this pull request to stack #2401
September 21, 2026 14:52
This was referenced Sep 22, 2026
AkramBitar
reviewed
Sep 22, 2026
AkramBitar
left a comment
AkramBitar
left a comment
Contributor
There was a problem hiding this comment.
Logic is correct and tests/vet are clean. Two issues undercut the fix, plus the self-flagged [ ] integration tests.
| } else { | ||
| t.metrics.DeletedTransactions.Add(1) | ||
| } | ||
| releaseLocks(ctx, t.logger, t.selectorManagerProvider, txID) |
Contributor
There was a problem hiding this comment.
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.
| if sm == nil { | ||
| return | ||
| } | ||
| if err := sm.Unlock(ctx, txID); err != nil { |
Contributor
There was a problem hiding this comment.
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").
AkramBitar
requested changes
Sep 22, 2026
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>
adecaro
force-pushed
the
fix/2395-sherdlock-antijoin-ordering
branch
from
September 22, 2026 15:20
655b435 to
0781e82
Compare
adecaro
force-pushed
the
fix/2395-release-locks-on-settlement
branch
from
September 22, 2026 15:21
1892c06 to
d3ef574
Compare
adecaro
added a commit
that referenced
this pull request
Sep 22, 2026
…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>
5 tasks done
Contributor
Author
adecaro
removed this pull request from stack #2401
September 23, 2026 05:08
AkramBitar
added a commit
that referenced
this pull request
Oct 2, 2026
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>
AkramBitar
added a commit
that referenced
this pull request
Oct 2, 2026
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>
AkramBitar
added a commit
that referenced
this pull request
Oct 2, 2026
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 productionUnlockcaller) was registeredsolely against
context.OnError—view.Contextexposes no success hook. Soa settled transaction's locks sat until the
sherdlocklease-expiry sweep(several minutes, by config), during which its tokens stayed invisible to the
Phase 4 anti-join and kept colliding.
locks once that status is known:
finality.Listener.runOnStatus(the livefinality subscription) and
TTXRecoveryHandler.applyFinalityLogic(recovery on restart) — for both
ConfirmedandDeleted, since afailed transaction will never spend the tokens it selected either.
finality.SelectorManagerProviderresolves thetoken.SelectorManagerbound to a fixed TMS, re-resolving it on every call rather than caching it —
mirroring the existing
TokenRequestHasher.ProcessTokenRequestpattern.Unlockis logged and does not fail thesettlement/recovery path (mirrors
Transaction.Release's existing style);the lease-expiry sweep remains the backstop for
Orphanconsumers (which thefinality path never observes) and for any release call that failed.
ttx.Service.Append,auditor.Service.Append(a harmless no-op unlock for the auditor role, whichnever 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) isstructurally blind to this mechanism: it calls
Selectand then immediatelydeletes/spends the tokens synchronously against the store, never routing
through the TTX finality settlement path where this PR's
Unlockhook lives.Re-running it before/after this change on the same environment gives identical
numbers:
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
Unlockfires exactly once per transaction, for bothConfirmedandDeleted, and that a failingUnlockdoes 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, andreframed "Lease expiry" as the backstop it now is rather than the sole release
mechanism.
Test plan
make lint-auto-fixcleanmake checkscleango test ./token/services/ttx/... ./token/services/auditor/... ./token/services/network/fabric/...(incl.-race) — all passFixes #2395