fix(selector): close sherdlock lock-contention gaps (Phases 3-8, #2395) by adecaro · Pull Request #2410 · LFDT-Panurus/panurus

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

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:

Anti-join — the DB query that feeds the selector now excludes already-locked tokens, so selectors stop queuing up to fight over the same row.
Amount-ordered candidates + shuffle — tokens are fetched smallest-first so a small payment doesn't wastefully grab a large token; equal-amount candidates are shuffled so contention doesn't just shift to whichever token happens to sort first.
Sufficiency-window randomization — when the smallest sufficient token is found, a bounded lookahead picks uniformly among similarly-sized candidates, preventing all concurrent selectors from deterministically targeting the exact same token.
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.
Immediate lock release on settlement — when a transaction reaches a terminal status (confirmed, deleted, or retry-exhausted), its locks are released immediately via the finality listener and recovery handler, rather than sitting until the 3-minute lease-expiry sweep during which those tokens remain invisible to all other selectors.
Postgres lock strategies — two new acquisition modes (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

Selection was still deterministic under realistic, distinct token amounts. The Phase 4 bucketedIterator/NewPermutation shuffle only randomizes within contiguous runs of byte-equal Quantity; 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 by sufficiencyWindow, magnitude-capped by maxSufficiencyRatio so it can't grab a wildly oversized token) and picks uniformly at random among the sufficient candidates in that window. New test TestSizeOrderedSelection_SufficiencyWindowShuffle proves real spread among several distinct, individually-sufficient amounts while the existing deterministic smallest-fit test still holds.
Listener.OnError never released locks, unlike OnStatus/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. OnError now calls the same releaseLocks helper as runOnStatus; new tests TestOnError_ReleasesLocks and TestOnError_LockReleaseErrorDoesNotPropagate cover it.

Follow-up: independent review, 6 fixes (TDD, each with its own commit)

Listener.runOnStatus's default: branch released locks on non-terminal network.Busy/Unknown statuses — these reach OnStatus in normal operation, not just error paths; releasing locks mid-commit let a concurrent Select hand the same tokens to a second transaction. Fixed by adding explicit case network.Busy, network.Unknown: that returns nil without touching locks or stores. New tests: TestOnStatus_DoesNotReleaseLocksOnNonTerminalStatus, TestOnStatus_ReleasesLocksOnceOnRetryExhaustion, TestOnStatus_ReleasesLocksExactlyOnceOnUnrecognizedStatus.
HasEnoughSpendableTokens fast-fail double-counted tokens the current call had already locked — the comparison was against remaining (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 full quantity. New test: TestSelectorFastFail_PartiallyFilledRequest.
The sufficiency-window shuffle was inert whenever the smallest sufficient token was already >5x the requested amount — the magnitude cap was computed from 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.
A batch-lock store error silently dropped the whole candidate window — contradicted its own comment by never requeuing the window tokens, costing a full refetch cycle and risking a transient store outage being misreported as lock contention. Fixed by refetching (bounded by the existing maxImmediateRetries budget). New tests: TestBatchLockStoreError_WindowIsRefetchedNotDropped, TestBatchLockStoreError_TerminatesWithinRetryBudget.
HasAnySpendableTokens was dead code — added to the public driver.TokenStore interface in Phase 4a, superseded by HasEnoughSpendableTokens in Phase 6, with zero remaining production callers. Removed from the interface, all implementations, and mocks.
Pre-existing lint breakage in benchmark_test.go — 4 ireturn + 1 thelper issue 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 nested x/token/services/network/evm module, which ./... from the root does not reach
go test -race ./token/services/selector/... ./token/services/ttx/... ./token/services/storage/... — 0 failures, no data races, including the Docker-backed TestHotTokenContention*/TestStaticHotTokenContentionPareto suites, the ordering/OnError tests, and the review-fix tests above
make checks — both halves green: checks-fast (license, gofmt, goimports, misspell, ineffassign, protos-lint, buf-format, ✓ All Go modules are tidy) and checks-heavy (go vet, go fix, staticcheck, govulncheck — the 2 reachable vulnerabilities are the ones already in govulncheck-allowlist.txt, treated as a pass as on main). This needed the staticcheck v0.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", while TestTMSScopedProviderWiringIsIntact (token/services/metricsdoc) greps the repo with filepath.WalkDir, which does not follow symlinks. Run from the worktree path, the only failure is the former; run through a …/panurus symlink, 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 named panurus.
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 T1 label across all three infra types (websocket, libp2p, replicas). The transaction … is not valid [Deleted] errors in the dlog log are asserted by the suite itself (integration/token/fungible/tests.go:550 passes "is not valid" as an expected message), not failures.

Fixes #2395

🤖 Generated with Claude Code

AkramBitar · 2026-09-24T13:02:22Z

Summary

The 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:

Selectors could see and fight over tokens already locked by someone else
All selectors always picked the same smallest token (deterministic ordering)
Lost lock races were retried in a tight loop on the same token for minutes
Settled transactions never released their locks, keeping tokens invisible to others

How:

DB query now hides locked tokens from the selector entirely
Candidates are shuffled within similarly-sized groups so concurrent selectors spread out
Tokens that lost a race are blacklisted for the rest of that call
Locks are released immediately on settlement, not after a 3-minute sweep
New Postgres lock modes that avoid server-side constraint violation errors
On Postgres, a whole window of candidate tokens is locked in one round trip instead of one at a time

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 stale_candidates_total stays at zero on that backend.

Addresses the five findings raised in review of #2410. metricsdoc: the LockStoreErrors counter was added to sherdlock's Metrics without regenerating testdata/metrics.golden, so TestMetricsReference failed on this branch - the SDK registered a metric the golden file did not list. docs/development/metrics.md already documented it, so only the generated file was stale; regenerated with UPDATE_GOLDEN=1. StubbornSelector: the spurious zero the review flagged is not reachable against current main. Its merged design threads one caller-owned attempted set through selectInternal and observes it through observeDistinctTokensAttempted, which skips an empty set, so a closed selector or an unparseable quantity - neither of which touches the fetcher or the locker - produces no observation at all, "not even zero" as Select's comment puts it. This branch's own -1 sentinel plumbing for the same guarantee was therefore dropped in favour of the upstream one while rebasing, and what it keeps is the part main lacks: ImmediateRetries accumulated across the backoff legs, since that metric has no set to union into and a caller wants the cost of the whole outer Select call rather than of its last leg. The two aggregate differently - distinct tokens union, retry events sum - which TestStubbornSelector_AggregatesRetryMetricsAcrossBackoff now pins: two legs contending the same token is a fan-out of one, not one count per leg. The two invalid-input regression tests that guard the skip are kept. Lookahead buffer: refreshCandidates drops s.pending whenever it installs a fresh cache, but its budget-exceeded early return happens before that, so the leg that gives up on SelectorSufficientButLockedFunds handed the buffer on intact. The whole point of the backoff that follows is to re-examine the world once other processes have had a chance to release their locks, so the next leg must start from a fresh fetch - instead it silently consumed candidates peeked from the pre-backoff snapshot, bypassing both the refetch and the fresh set's randomized window. selectInternal now clears s.pending on entry; dropping is lossless, since the cache those candidates came from is still installed and re-offers them once it exhausts. Blacklisted candidates: nextCandidate let tokens this call has already lost a lock race on into the sufficiency window. They cannot be locked at all, so this spent part of the randomized pick on a guaranteed no-op - diluting the very contention spread the window exists to provide - and re-buffered the blacklisted pick into s.pending, where the next call built another window around it and skipped it again. A scan over a fully blacklisted cache therefore re-walked it once per candidate: 42 cache reads instead of 33 in the test's 3-token wallet. A blacklisted candidate is now returned straight away with no lookahead, leaving selectInternal's blacklist branch the single owner of the tokensLockedByOthersExist bookkeeping, and dropped rather than re-buffered while growing a window - mirroring what selectInternal's own batch-window loop already does. Which tokens end up selected is unchanged: the iterator yields ascending amounts, so a blacklisted anchor's threshold can never exceed the next free anchor's, and it cannot pull in a token the skip would exclude. What changes is wasted work and the pick distribution, so the regression test asserts the work bound. created_at scanning: scannableTime exists because sqlite hands back created_at as raw text - the shared schema declares it TIMESTAMPTZ, which is not one of the DATE/DATETIME/TIMESTAMP names modernc.org/sqlite converts to a native time.Time. But which text it is depends on the writer and on the driver's _time_format, and the scanner pinned itself to the single layout modernc happens to default to, so ListLocks failed outright on any value that deviated - most sharply on a time.Time still carrying a monotonic reading, whose String() form gains a trailing " m=+<seconds>" that no layout matches. That shape is not reachable today: LockAt binds createdAt.UTC() and Time.UTC() drops the reading, while the Postgres tryInsertOnConflict path never goes through the sqlite text encoding at all. This is hardening rather than a live bug fix - a reader that breaks on the one shape a forgotten .UTC() produces, in a diagnostic path whose whole job is to report lock age, is needlessly brittle. parseSQLTime now strips the monotonic suffix and tries the layouts in sqliteTimeLayouts in turn, covering the abbreviation-less zone form, RFC 3339 and zone-less timestamps alongside modernc's default. Also fixes an unrelated lint break inherited from c77b1f4: testifylint's float-compare rule rejects assert.Equal on a float64, so the two LockStoreErrors assertions failed `make lint`. Switched to assert.InEpsilon with the 0.0001 epsilon the repo already uses (token/services/auditor/config_test.go). docs/services/selector.md documents the two new sufficiency-window rules. Signed-off-by: AkramBitar <akram@il.ibm.com>

Effi-S

Only Minor / Nit findings:

Also to consider:

No index on amount (tokens.go).
New ORDER BY amount on 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.

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>

AkramBitar · 2026-10-05T11:29:36Z

All four addressed in 9b8ecd3 (branch squashed to one commit).

1. simple bounded-pool deadlock — fixed, not just confirmed. The overlap turned out to be unnecessary: the scan loop is already done with the cursor by the time concurrencyCheck runs, and a retry opens a fresh one. selectByID now closes it before the re-check, so one in-flight Select holds one connection instead of two. TestSimpleDriverBoundedPool (16 selectors, MaxOpenConns=2) stalls if the fix is reverted.

2. IsTerminalStatus test-only — I don't think this holds. cmd/tokendiag/cobra/locks/runner.go:64 is a production caller, so I left it exported. Say the word if you meant a different symbol.

3. bucketedIterator bucketing — comment added on NewPermutation: string equality identifies equal amounts only because the quantity encoding is canonical, while the ORDER BY is numeric. A non-canonical encoding would degrade the shuffle to a no-op rather than break the ordering.

4. amount index — added. idx_spendable_amount on (owner_wallet_id, token_type, amount), partial on the spendable query's own predicates. New index name rather than an extra column on idx_owner_wallet_part, since CREATE INDEX IF NOT EXISTS won't replace an index already deployed under that name.

make checks and make lint clean; selector, sql/common, sql/sqlite and the Postgres token suites pass on the current base. Docs updated in docs/services/selector.md.

Effi-S

@AkramBitar,
Some Low findings:

common/tokens.go:GetSchema and common/tokenlock.go:GetSchema both CREATE TABLE IF NOT EXISTS
the TokenLocks table. Definitions are currently identical, but IF NOT EXISTS means 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 as LockConflicts and stale_candidates_total stays 0 in production.
!hasEnough now returns terminal SelectorInsufficientFunds with 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..
SpendableTokensIteratorBy reads 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>

AkramBitar · 2026-10-05T13:28:02Z

Thanks @Effi-S — all three in b7c9890, squashed to one commit.

1. Duplicate TokenLocks DDL. Fixed. Both stores have to emit the table, so they now share one builder (common.tokenLocksSchema): a change reaches both or neither.

2. stale_candidates_total stays 0. Fixed rather than documented. LockBatch returns driver.BatchLockOutcome{Won, Stale} and the claim classifies every candidate in the same round trip, so the batch path counts stale drops, keeps them out of tokensLockedByOthersExist, and refreshes the candidate cache — the same recovery the single-token path had.

One thing worth your eye: 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, not stale. Mutation-tested — classifying off the claim's source fails the skipLocked test on exactly that assertion. A backend that cannot classify may still leave Stale empty and keeps today's behaviour.

3. !hasEnough on a lagging replica. No change, and I think the premise does not hold — happy to be wrong. On origin/main that branch already returned terminal SelectorInsufficientFunds unconditionally, and that error exits StubbornSelector's backoff loop, so lag failed such a call before this PR too. The check only ever turns a give-up into a retry. Reasoning recorded at the call site and in docs/services/selector.md.

New real-Postgres test covers won/stale/contended in one claim under both batch strategies; make checks and make lint clean.

adecaro added this to the Q3/26 milestone Sep 23, 2026

adecaro added bug Something isn't working testing db storage token-selector labels Sep 23, 2026

adecaro self-assigned this Sep 23, 2026

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

adecaro marked this pull request as ready for review September 23, 2026 05:06

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

adecaro force-pushed the fix/2395-consolidated-3-8 branch 2 times, most recently from 22c893d to 1bb5da2 Compare September 23, 2026 08:00

adecaro mentioned this pull request Sep 23, 2026

Alert/runbook for static-hot-token lock contention (follow-up to #2395) #2413

Open

adecaro force-pushed the fix/2395-consolidated-3-8 branch from 1bb5da2 to 0b47be8 Compare September 23, 2026 08:44

AkramBitar requested review from Effi-S and alexandrosfilios September 24, 2026 13:14

Effi-S requested changes Sep 27, 2026

View reviewed changes

Base automatically changed from fix/2395-sherdlock-lock-contention-diagnostics to main September 29, 2026 12:40

AkramBitar force-pushed the fix/2395-consolidated-3-8 branch from 3517dda to 790ec0a Compare September 29, 2026 15:15

AkramBitar force-pushed the fix/2395-consolidated-3-8 branch from 790ec0a to b856a75 Compare September 30, 2026 09:59

AkramBitar force-pushed the fix/2395-consolidated-3-8 branch 3 times, most recently from 661291a to 583e600 Compare October 2, 2026 11:18

AkramBitar requested a review from Effi-S October 2, 2026 11:23

Effi-S requested changes Oct 4, 2026

View reviewed changes

Comment thread token/services/selector/simple/selector.go

Comment thread token/services/storage/db/driver/token.go

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

Effi-S force-pushed the fix/2395-consolidated-3-8 branch from 583e600 to db0cb20 Compare October 4, 2026 15:34

AkramBitar force-pushed the fix/2395-consolidated-3-8 branch from db0cb20 to 9b8ecd3 Compare October 5, 2026 11:28

AkramBitar requested a review from Effi-S October 5, 2026 11:32

Effi-S reviewed Oct 5, 2026

View reviewed changes

AkramBitar force-pushed the fix/2395-consolidated-3-8 branch from 9b8ecd3 to b7c9890 Compare October 5, 2026 13:26

Navigation Menu

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

fix(selector): close sherdlock lock-contention gaps (Phases 3-8, #2395)#2410
adecaro wants to merge 1 commit into
mainfrom
fix/2395-consolidated-3-8

adecaro commented Sep 23, 2026 •

edited by AkramBitar

Loading

Uh oh!

AkramBitar commented Sep 24, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Effi-S left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AkramBitar commented Oct 5, 2026 •

edited

Loading

Uh oh!

Effi-S left a comment

Uh oh!

AkramBitar commented Oct 5, 2026 •

edited

Loading

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

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

Uh oh!

Summary

Follow-up: two review-flagged gaps closed

Follow-up: independent review, 6 fixes (TDD, each with its own commit)

Test plan

Uh oh!

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

Uh oh!

Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Effi-S left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AkramBitar commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Effi-S left a comment

Choose a reason for hiding this comment

Uh oh!

AkramBitar commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adecaro commented Sep 23, 2026 •

edited by AkramBitar

Loading

AkramBitar commented Sep 24, 2026 •

edited

Loading

AkramBitar commented Oct 5, 2026 •

edited

Loading

AkramBitar commented Oct 5, 2026 •

edited

Loading