Repository navigation
Conversation
22c893d to
1bb5da2
Compare
1bb5da2 to
0b47be8
Compare
SummaryThe PR fixes a bug where a small number of tokens were getting "stuck" under concurrent load — many selectors would pile up racing to lock the same few tokens, causing spurious "insufficient funds" errors. Root causes fixed:
How:
One thing to know when monitoring: on Postgres the batch lock only reports which tokens it won, so a token that was already spent looks the same as one that was simply locked by someone else. It is counted as a lock conflict, which means |
661291a to
583e600
Compare
Effi-S
left a comment
Effi-S
left a comment
There was a problem hiding this comment.
Only Minor / Nit findings:
Also to consider:
- No index on
amount(tokens.go).
NewORDER BY amounton every spendable-tokens query sorts without a supporting index. Cached fetcher masks it; could matter for large wallets / lazy fetcher. Consider an index if this query is hot.
583e600 to
db0cb20
Compare
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>
db0cb20 to
9b8ecd3
Compare
|
All four addressed in 9b8ecd3 (branch squashed to one commit). 1. 2. 3. 4.
|
Effi-S
left a comment
Effi-S
left a comment
There was a problem hiding this comment.
@AkramBitar,
Some Low findings:
common/tokens.go:GetSchemaandcommon/tokenlock.go:GetSchemabothCREATE TABLE IF NOT EXISTS
the TokenLocks table. Definitions are currently identical, butIF NOT EXISTSmeans if they ever
drift, whichever runs first silently wins and the other is a no-op- Postgres is the sole
BatchLocker, and the batch path can't distinguish stale from lost-race, so it
books everything asLockConflictsandstale_candidates_totalstays 0 in production. !hasEnoughnow returns terminalSelectorInsufficientFundswith no retry. On a lagging read replica
a wallet whose just-added tokens aren't visible yet fast-fails instead of retrying. Not a regression..
SpendableTokensIteratorByreads the same replica, so the old path would also have failed (afterburning retries).. but the retry cushion is gone.
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>
9b8ecd3 to
b7c9890
Compare
|
Thanks @Effi-S — all three in 1. Duplicate 2. One thing worth your eye: under 3. New real-Postgres test covers won/stale/contended in one claim under both batch strategies; |
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>
b7c9890 to
b46c93a
Compare
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>
b46c93a to
d2f1599
Compare
There was a problem hiding this comment.
Findings
F1 — Auditor finality listener now releases "selection locks" on every finalized tx, though an auditor performs no selection — Low
- File / line:
token/services/ttx/finality/listener.go:228(thereleaseLocks(ctx, …)
call at the end ofrunOnStatus), wired for the auditor at
token/services/auditor/auditor.go:295. - What:
runOnStatusunconditionally callsreleaseLocks→SelectorManager().Unlock(ctx, txID)
on every terminal status (Confirmed/Deleted). This is exactly right for the owner
path (ttx — mechanism 4, the whole point of the fix). But the auditor listener is wired
with the same provider (auditor.go:295), and an auditor never acquires token-selection
locks for the transactions it audits. - Failure scenario: for every transaction the auditor finalizes, it resolves the TMS
selector manager and issues aDELETE FROM <TokenLocks> WHERE consumer_tx_id = <txID>that
matches zero rows — one wasted round trip on a hot path. Worse case: if the auditor's TMS
has no usableSelectorManager,releaseLockslogs aWARNper finalized transaction
("failed to get selector manager to release locks …"/"failed to release locks …"),
i.e. steady-state log noise. - Background: mechanism 4 (#2395) is about the spending node leaking locks it took during
selection. The auditor is a different role; it was wired in alongside ttx/fabric/evm for
uniformity. Consider gating the release on "this node actually selects" (owner path only), or
confirm the auditor'sSelectorManager()resolves cheaply and silently to a no-op so there's
no per-tx warning.
F2 — New Selector.pending field is read/written without the mutex that guards the sibling cache field — Informational / Low
- File / line:
token/services/selector/sherdlock/selector.go:77(field decl),
with unsynchronized access at:168(s.pending = nil),:506(refreshCandidates),
and:522–529(dequeue) /:605,:624(nextCandidate). - What:
s.cacheis protected bys.muspecifically so a concurrentClose()can't swap
the iterator out mid-read (next()holdss.mu). The newpending []*UnspentTokenInWallet
slice is mutated vias.pending = s.pending[1:]/append(...)with no lock. The code
comment states onlyselectInternal's single goroutine touches it. - Failure scenario: only if the same per-
txID-cachedSelector(the manager caches one
instance pertransaction.ID,manager.go:57) ran twoSelectcalls concurrently — then
the slice ops race (and aClose()concurrent with apendingread is also unsynchronized,
unlike thecacheread). - Background: this is not a regression — concurrent
Selecton oneSelectorwould
already corrupt thecacheiteration logically even with the mutex (the mutex only protects
the swap, not iteration coherence), so the single-Select-per-selector assumption predates
this PR. Noted only becausependingis a newly added shared field that, unlikecache, has
no memory-safety guard at all. If concurrent same-txIDselection is ever possible, both
fields need rethinking; if it is contractually impossible, a one-line assertion/comment at
the type would make the invariant explicit.
F3 — Unrecognized (default) finality status now triggers lock release after retry exhaustion — Informational
- File / line:
token/services/ttx/finality/listener.go:207–211(thedefaultbranch
returning an error) →:139(OnStatusreleasing locks once the retry budget is spent). - What: before this PR an unknown status just errored and was retried/logged. Now, after
MaxRetryexhaustions,OnStatusreleasestxID's locks. TheBusy/Unknownstatuses are
correctly carved out as non-terminal (:197–206) and do not release — good. But a
genuinely unrecognized status (neitherValid/Invalid/Busy/Unknown) will, after
retries, release locks for a transaction whose true state is unknown. - Failure scenario: a never-before-seen status code on a still-in-flight tx → locks freed
early → anotherSelectmay grab those tokens. Double-spend is still prevented by the
row-levelINSERTbackstop, theis_deletedpredicate, and the in-same-statement
spendability check, so the worst case is a spurious selection that fails later — not an
incorrect ledger. - Background: this is a should-never-happen path (the comment acknowledges it). Flagged
only so the behavioral change from "retry forever-ish then log" to "release locks" is a
conscious choice; it is defensible given the lower-layer backstops.
🤖 Generated with Claude Code
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>
d2f1599 to
def3585
Compare
|
Thanks @Effi-S — all three in F1 — auditor releasing "selection locks" it never took. Fixed, and in two places rather than one. You were right that the auditor was wired in for uniformity and that the release is the spending node's business: Checking the rest of that wiring turned up the same bug a second time, which your finding did not name: the evm driver starts a recovery sweep over both the transaction store and the audit store ( F2 — Folding the buffer into the lock also closed a gap that was not in the finding: F3 — unrecognized status releasing locks after retry exhaustion. Changed, and in the direction your "conscious choice" framing implies it should go: it is not defensible enough to keep. The lower-layer backstops do prevent an incorrect ledger, but the status is unclassifiable precisely because nothing is known about where the transaction stands — it may well still be in flight — and that is the same situation The same sentinel removes a second thing that bought nothing: that error terminates the retry loop on the first attempt, since calling Tests: the two unrecognized-status listener tests are inverted to the kept-locks behaviour, plus one pinning that the status is not retried; an auditor wiring test asserting a finalized transaction resolves no selector manager at all; and a
One unrelated find while running |
Effi-S
left a comment
Effi-S
left a comment
There was a problem hiding this comment.
2 More findings:
- Low: Batch path loses committed locks on a mid-
RETURNINGread failure
Where: token/services/selector/sherdlock/selector.go:406:407 (the if lockErr != nil
branch after TryLockBatch); the error originates in
token/services/storage/db/sql/postgres/tokenlock.go:301 / :316 (claimCandidates returning
driver.BatchLockOutcome{}, err from a row-scan / rows.Err() failure).
What: claimCandidates runs one WITH … INSERT … RETURNING statement: the lock rows are
committed server-side (auto-commit) and then the client reads the result set. If the connection
drops after the commit but during the read, QueryContext/rows.Scan/rows.Err()
returns an error and claimCandidates returns an empty outcome plus that error. In
selectInternal, the lockErr != nil branch (:407) logs, bumps LockStoreErrors, charges a
retry and refreshCandidates — discarding outcome entirely. The tokens were really locked
(rows exist with our consumer_tx_id), but they're never added to selected.
How it fits / blast radius: The just-locked tokens now have TokenLocks rows, so the
notLocked anti-join (mechanism 3) hides them from this call's refetch and from every other
selector. They can't be re-selected. If the overall Select then succeeds via other tokens,
selectWithoutMetrics only calls UnlockAll on an error return, so these stray locks are not
cleaned up by this call — they persist until this same tx settles (mechanism 4's
Unlock(txID) deletes all rows for the consumer_tx_id, strays included) or the lease-age
sweep. So it is self-healing at settlement, not a permanent leak, and requires a precise
connection failure window — hence Low.
Why it's still worth noting: It's an asymmetry with the single-token path, which deliberately
handles the equivalent ambiguous case: common/tokenlock.go LockAt treats a RowsAffected
error as "lock acquired" (comment: "treat the lock as acquired rather than failing a selection
on a driver capability") and the token is added to selected. The batch path makes the
opposite choice silently.
Suggested direction: On a read error from claimCandidates, the won set is unknown but may be
non-empty; either surface that ambiguity so the caller can UnlockAll defensively, or treat the
batch like the single-token path treats its ambiguous outcome.
- Low: Batch path loses committed locks on a mid-
RETURNINGread failure
Where: token/services/auditor/auditor.go:288 constructs finality.NewListener(...) with
finality.NewSelectorManagerProvider(a.tmsProvider, a.tmsID) at :295 — the same Listener
type the ttx sender wires at token/services/ttx/db.go:70.
What: releaseLocks now fires on every terminal status the auditor's listener sees, calling
a's SelectorManager().Unlock(txID) → DELETE FROM <auditor TokenLocks> WHERE consumer_tx_id = txID. The auditor audits transactions; it does not run selection and holds no selection locks for
those txIDs. So the DELETE matches zero rows every time.
How it fits / blast radius: Pure overhead, not a correctness issue — the auditor's TMS has its
own table names, so this can't touch the sender's locks. Cost is one wasted DB round trip per
audited terminal tx, plus a WarnfContext line per tx if an audit-only TMS's
SelectorManager() returns an error (releaseLocks logs and continues on error). On a busy
auditor that's measurable log noise and load.
Suggested direction: Gate the release wiring to the sender path, or make releaseLocks a
no-op when the resolved SelectorManager has nothing to unlock (it already tolerates a nil
manager — the auditor just doesn't return nil).
| } | ||
| } | ||
|
|
||
| // OnError is called when a finality event for txID could not be delivered after all retries. |
There was a problem hiding this comment.
Medium
OnError releases a transaction's selection locks unconditionally. The OnStatus
retry-exhausted path does the same for whatever failed inside runOnStatus (including the
default branch) , which errors on an unrecognized status that retrying can
never reclassify.
Failure scenario: The finality framework gives up delivering a status for a tx that is still
committing on-ledger (e.g. repeated Busy, then the delivery machinery exhausts its own retries
and calls OnError). releaseLocks unlocks the tx's inputs; a concurrent Select can then pick
those still-live tokens for a second transaction. The ledger will reject the eventual
double-spend, so this is wasted/invalid work rather than an on-ledger loss — but it re-opens
exactly the window the Busy/Unknown guard was added to close.
Suggested direction: OnError has no status to key on, so it cannot know the tx is terminal;
consider not releasing there (let settlement / recovery / the lease sweep do it), or release only
when the tx's stored status is already terminal. For OnStatus, route the unrecognized-default
case the way recovery.go does (no release) rather than through the give-up release.
Effi-S
left a comment
Effi-S
left a comment
There was a problem hiding this comment.
Findings
| # | Severity | File:line | Summary |
|---|---|---|---|
| 1 | Medium | token/services/ttx/finality/listener.go:115, :139 |
Locks released unconditionally on OnError / retry-exhaustion, even for a possibly in-flight tx — contradicting the Busy/Unknown guard the PR itself adds. |
| 2 | Low | token/services/selector/sherdlock/selector.go:407 (+ postgres/tokenlock.go:301,316) |
Batch path discards outcome when the RETURNING read fails after the lock rows committed, so won locks are held but never put in selected. |
| 3 | Low | token/services/auditor/auditor.go:295 |
The shared Listener now issues an Unlock(txID) per terminal audited tx against the auditor's lock store, which never holds those locks — wasted round trip + possible per-tx warn log. |
| 4 | Info | token/services/selector/sherdlock/selector.go:320 (whole selectInternal) |
Architectural: randomized smallest-fit covering selection is implemented as stateful app-side machinery rather than pushed into the DB layer. Not a defect. |
| 5 | High | token/services/storage/tokenlockdb/store.go:21 (effect at sherdlock/selector.go:835) |
The entire batch-locking path (Phase 5) is dead in production. The wrapper actually wired into NewSherdSelector embeds the driver.TokenLockStore interface, which has no LockBatch, so the lockDB.(BatchLocker) assertion always fails and supportsBatch is never true. Proven by compiler. |
| 6 | Medium | token/services/selector/sherdlock/fetcher.go:441 (+ :566) |
Cache-invalidation TOCTOU: an in-flight update() that read its snapshot before InvalidateCache() still installs that pre-invalidation snapshot and stamps it fresh, silently losing the stale-candidate refresh for ~one freshness interval. |
| 7 | Low | token/services/ttx/finality/listener_test.go:598, :526; postgres/*_test.go; testutils/test_cases.go:418 |
Test gaps: TestCheckTokenRequest exercises the stdlib, not the SUT; TestOnError asserts nothing; the Postgres strategy tests silently skip with no DB; a copied error-collector goroutine has a latent drop-errors race. |
…nknown status (#2395) Second review round on #2410, six fixes. The batch path (mechanism 6) was unreachable in every deployment. The manager's Locker is a *tokenlockdb.StoreService, which embeds the driver.TokenLockStore interface; embedding an interface promotes only its method set, and no driver interface declares LockBatch, so the lockDB.(BatchLocker) assertion always failed. StoreService.Unwrap (nil-receiver safe) exposes the wrapped store and sherdlock.asBatchLocker looks at both the value and the store it wraps. The old conformance assertion named *postgres.TokenLockStore, a type the selector is never handed, so it is replaced by tests over the real wrapper, covering the batch-incapable and both nil cases. OnError released the locks of a transaction whose status is unknown. The EVM driver calls it precisely when no verdict could be obtained, so it now keeps them; the recovery and lease-expiry sweeps are the backstops that need no verdict. The shared TokenLocks DDL ran under two different Postgres advisory locks: prefixSchemaWithLocks takes them in ascending, deduplicated order - so sharing a subset cannot deadlock - and the token store now takes the TokenLocks lock as well as its own. The Fabric audit-store recovery handler still wired the real selector manager provider, the defect fixed on the EVM side last round; createRecoveryManager takes the provider as a parameter. The batch window was uncapped, so a dust-heavy wallet could exceed Postgres's 65535 bind-parameter limit; maxBatchWindow caps it at 256 and the outer loop claims another window when needed. A detached godoc comment on SpendableTokensIteratorBy is reattached. Signed-off-by: AkramBitar <akram@il.ibm.com>
…ial batch locks in use (#2395) Third review round on #2410: the three findings that were still open. An update() whose store read began before InvalidateCache() installed its snapshot anyway and stamped it fresh, so the invalidation was lost for a whole freshness interval. The only re-check before installing was staleness, which an invalidation satisfies by construction. cachedFetcher now carries an invalidations counter, read under the lock before the snapshot is taken and re-read before it is installed; a snapshot that predates an invalidation is discarded with lastFetched left at zero, so readers keep seeing the old snapshot - the documented InvalidateCache contract - and the next read refetches, after the invalidation. A batch claim is a single statement that has committed by the time its result set is read, so a failure mid-read leaves lock rows held by the consumer tx. claimCandidates returned an empty outcome in that case and the selector discarded it, which held those locks, unusable and unreported, until the lease-expiry sweep. The store now returns the verdicts it did read alongside the error and the selector's store-error branch harvests outcome.Won into selected, resetting the immediate-retry budget exactly as the success path does, before refetching the unread remainder; a harvest that covers the request returns. The contract is spelled out on driver.BatchLockOutcome and sherdlock.BatchLocker. Test gaps, four of them. TestCheckTokenRequest asserted on encoding/base64 rather than on the guard, which is unexported and so unreachable from finality_test: it is now a table-driven internal test that runs every case through both Listener.checkTokenRequest and TTXRecoveryHandler.checkTokenRequest, so the two copies cannot drift. TestOnError asserted nothing and now pins what OnError must not do for a transaction with no verdict: no SetStatus, no NewTransaction, no NotifyStatus. The Postgres lock-strategy tests skipped silently without a server; a shared startPostgres helper names the reason and the variable that changes it, and fails rather than skips when PANURUS_REQUIRE_POSTGRES is set, so an environment that is supposed to have Docker cannot report green with the strategy tests never run. And the three parallelSelect* helpers collected per-worker errors behind a mutex their collector goroutine held for its lifetime, with nothing ordering that goroutine before the caller, which could take the mutex first and return the still-empty slice - an errorCollector whose Wait closes the channel and waits for the drain replaces it. Both behavioural fixes were verified to fail against the pre-fix code (TestCachedFetcher_DiscardsSnapshotInvalidatedMidRead, TestBatchLockPartialOutcome_TakesTheWonLocks). Review finding 4 of that round is an architectural note on selectInternal, explicitly not a defect, and is left as it stands. Signed-off-by: AkramBitar <akram@il.ibm.com>
…and partial batch claims The cache-refresh section described only the time- and query-based triggers, so the explicit InvalidateCache path - and the reason it needs more than clearing a timestamp, since a refresh already reading from the store would otherwise install its pre-invalidation snapshot and stamp it fresh - went unmentioned. The immediate-retry paragraph likewise described only a batch claim that fails outright, not one that fails after committing, whose reported winners the selector keeps rather than abandoning their lock rows to the lease sweep. Signed-off-by: AkramBitar <akram@il.ibm.com>
…ates The comment added with the partial-outcome harvest claimed the refetch makes the unclassified remainder visible to the next scan. It does not: a candidate this claim actually won has a TokenLocks row, which the notLocked anti-join then hides from the refetch and from every other selector, so this call cannot recover it. It is released when the transaction settles - Unlock(txID) deletes every row for the consumer tx, strays included - with the lease-expiry sweep behind that. Signed-off-by: AkramBitar <akram@il.ibm.com>
|
Thanks @Effi-S — this covers the two reviews that hadn't been answered yet, and corrects something I got wrong in my previous comment. Pushed as four commits on top of Correction to my last commentI wrote that the Fabric driver "only wires the transaction store, so it was already correct". That was wrong. Review 5442615457 (2 findings)1 — batch path loses committed locks on a mid- Your blast-radius analysis is what shaped the fix, including the limit of it: candidates the statement won but never reported are still unrecoverable by this call, because the 2 — the auditor's listener unlocking locks it never took. Fixed in the previous round ( Review 5443625924 (7 findings)
On #5, you were right that the compiler proves it: embedding On #6, the invalidation was lost rather than merely delayed: the only pre-install re-check was staleness, which an invalidation satisfies by construction, so a snapshot read before the invalidation was installed and stamped fresh. On #7, all four: On #4, agreed that it is not a defect, and I am not changing it in this PR. The randomized smallest-fit window is app-side because two of its inputs are per- VerificationBoth behavioural fixes have regression tests verified to fail against the pre-fix code: Ready for another round. |
| @@ -469,7 +470,9 @@ func (n *Network) connect(ns string) ([]token2.ServiceOption, error) { | |||
| if err != nil { | |||
| return nil, errors.Wrapf(err, "failed to get audit storage for [%s]", tmsID) | |||
| } | |||
There was a problem hiding this comment.
Severity: Nit
createRecoveryManager unconditionally wires the recovery handler with the
real ttxfinality.NewSelectorManagerProvider(...) (line 517)
| if errors.Is(err, ErrUnrecognizedStatus) { | ||
| // The transaction's real state is unknown, so its locks stay held: see | ||
| // ErrUnrecognizedStatus. | ||
| return |
There was a problem hiding this comment.
Severity: Low
When the backoff select in RunWithErrorsContext observes a cancelled context
it returns ctx.Err() (token/services/utils/retry.go:185-186). Back in OnStatus, that error
is neither nil nor ErrUnrecognizedStatus, so control falls through the if errors.Is(err, ErrUnrecognizedStatus) guard (line 148) to the unconditional releaseLocks(newCtx, ...) at line
160. A context cancellation (shutdown / interruption) is thus classified identically to
"recognized terminal status whose local persistence kept failing", and a release is attempted
even though the transaction's true state was never established.
This branch has not been deployed
3 participants
Summary
Fixes the sherdlock token selector's lock-contention hot-spot that caused spurious "insufficient funds" errors under concurrent load (#2395).
Six mechanisms made a small number of "hot" tokens absorb the vast majority of lock collisions. This PR closes all of them:
Select()call, instead of being retried in a tight loop for minutes.onConflict,skipLocked) that avoid server-side unique-constraint violations on lost races, plus batch locking (LockBatch) that claims a covering window of candidates in a single round-trip.Consolidates the #2398-#2403 stack (Phases 3-8 of #2395) into a single PR on top of #2397's diagnostics/baseline, so the remaining lock-contention work reviews as one unit instead of five stacked PRs. Supersedes #2398, #2399, #2400, #2402, #2403.
Follow-up: two review-flagged gaps closed
bucketedIterator/NewPermutationshuffle only randomizes within contiguous runs of byte-equalQuantity; with mostly-distinct amounts (the CERT-incident shape) every bucket is size 1, so every concurrent selector always targeted the single smallest sufficient token — reproducing the exact hot-token pattern the issue warned against. Fixed in the selector layer (sherdlock/selector.go): when the ascending candidate scan reaches a token that alone covers the remaining requested amount, it now looks ahead over a small bounded window (count-capped bysufficiencyWindow, magnitude-capped bymaxSufficiencyRatioso it can't grab a wildly oversized token) and picks uniformly at random among the sufficient candidates in that window. New testTestSizeOrderedSelection_SufficiencyWindowShuffleproves real spread among several distinct, individually-sufficient amounts while the existing deterministic smallest-fit test still holds.Listener.OnErrornever released locks, unlikeOnStatus/recovery — a transaction whose finality notification is permanently undeliverable (retry budget exhausted) kept its locks held until the next lease-expiry sweep, reproducing Phase 5's original gap via a different trigger.OnErrornow calls the samereleaseLockshelper asrunOnStatus; new testsTestOnError_ReleasesLocksandTestOnError_LockReleaseErrorDoesNotPropagatecover it.Follow-up: independent review, 6 fixes (TDD, each with its own commit)
Listener.runOnStatus'sdefault:branch released locks on non-terminalnetwork.Busy/Unknownstatuses — these reachOnStatusin normal operation, not just error paths; releasing locks mid-commit let a concurrentSelecthand the same tokens to a second transaction. Fixed by adding explicitcase network.Busy, network.Unknown:that returnsnilwithout touching locks or stores. New tests:TestOnStatus_DoesNotReleaseLocksOnNonTerminalStatus,TestOnStatus_ReleasesLocksOnceOnRetryExhaustion,TestOnStatus_ReleasesLocksExactlyOnceOnUnrecognizedStatus.HasEnoughSpendableTokensfast-fail double-counted tokens the current call had already locked — the comparison was againstremaining(quantity minus what this call already selected), but the lock-ignoring total already includes the already-selected tokens, so the check could never fire once anything had been won. Fixed by comparing against the fullquantity. New test:TestSelectorFastFail_PartiallyFilledRequest.remaining, not from the anchor token itself, so the window collapsed to size 1 exactly in the regime it was built to fix. Fixed by anchoring the cap on the anchor token's own quantity. New test:TestSizeOrderedSelection_SufficiencyWindowWhenEveryTokenDwarfsTheRequest.maxImmediateRetriesbudget). New tests:TestBatchLockStoreError_WindowIsRefetchedNotDropped,TestBatchLockStoreError_TerminatesWithinRetryBudget.HasAnySpendableTokenswas dead code — added to the publicdriver.TokenStoreinterface in Phase 4a, superseded byHasEnoughSpendableTokensin Phase 6, with zero remaining production callers. Removed from the interface, all implementations, and mocks.benchmark_test.go— 4ireturn+ 1thelperissue that would have failed CI. Fixed mechanically.Test plan
All items verified locally against the rebased branch (Go 1.27.1, Docker 29.8.1, Fabric v3.1.1 binaries).
go build ./...— plus the nestedx/token/services/network/evmmodule, which./...from the root does not reachgo test -race ./token/services/selector/... ./token/services/ttx/... ./token/services/storage/...— 0 failures, no data races, including the Docker-backedTestHotTokenContention*/TestStaticHotTokenContentionParetosuites, the ordering/OnError tests, and the review-fix tests abovemake checks— both halves green:checks-fast(license, gofmt, goimports, misspell, ineffassign, protos-lint, buf-format,✓ All Go modules are tidy) andchecks-heavy(go vet,go fix,staticcheck,govulncheck— the 2 reachable vulnerabilities are the ones already ingovulncheck-allowlist.txt, treated as a pass as onmain). This needed thestaticcheckv0.7.0 → v0.8.1 bump in this PR: v0.7.0 panics in its own IR builder under a Go 1.27.1 module.make unit-tests-race(full suite) — every package green, with no data races and no panics. It takes two runs to show this, because two pre-existing tests have contradictory path requirements and neither can be satisfied at once outside CI:TestTranslatePath(token/services/identity/config) asserts the translated path contains"panurus", whileTestTMSScopedProviderWiringIsIntact(token/services/metricsdoc) greps the repo withfilepath.WalkDir, which does not follow symlinks. Run from the worktree path, the only failure is the former; run through a…/panurussymlink, the only failure is the latter; each passes in the other path, so the union covers every package. CI hits neither, since its checkout is a real directory namedpanurus.Integration tests (fabtoken/dlog,
TEST_FILTER="T1") — both suites pass against this branch:make integration-tests-fabtoken-fabric-t1→Ran 3 of 15 Specs— 3 Passed, 0 Failed, 12 skipped (33m55s)make integration-tests-dlog-fabric-t1→Ran 3 of 44 Specs— 3 Passed, 0 Failed, 41 skipped (35m18s)The 3 specs per suite are the
T1label across all three infra types (websocket, libp2p, replicas). Thetransaction … is not valid [Deleted]errors in the dlog log are asserted by the suite itself (integration/token/fungible/tests.go:550passes"is not valid"as an expected message), not failures.Fixes #2395
🤖 Generated with Claude Code