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

cmd/tokendiag/cobra/locks/runner.go

@@ -18,24 +18,6 @@ import (
 	"github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors"
 )
 
-// isTerminal reports whether status is a terminal status of a consuming transaction —
-// i.e. one after which the lock it holds should already have been released. A lock
-// still present with a terminal-status consumer is the mechanism-4 leak from #2395:
-// nothing on the success path calls UnlockByTxID, so the row survives until the
-// next lease-age sweep.
-func isTerminal(status *driver3.TxStatus) bool {
-	if status == nil {
-		return false
-	}
-
-	switch *status {
-	case driver3.Confirmed, driver3.Deleted, driver3.Orphan:
-		return true
-	default:
-		return false
-	}
-}
-
 // statusName renders status for display, or "unknown" if nil.
 func statusName(status *driver3.TxStatus) string {
 	if status == nil {
@@ -79,7 +61,7 @@ func Run(ctx context.Context, w io.Writer, stores *Stores, now time.Time) error
 			oldest = age
 		}
 		terminalMark := ""
-		if isTerminal(r.Status) {
+		if driver3.IsTerminalStatus(r.Status) {
 			leaked++
 			terminalMark = "  [LEAKED: consumer is terminal, lock should have been released]"
 		}

docs/development/metrics.md

@@ -184,7 +184,38 @@ to distinguish "one hot token retried many times" from "many tokens each contend
 `lock_conflicts_total` is deliberately unlabeled by token id or wallet id to avoid unbounded
 cardinality — per-token attribution belongs in the selector's debug-level log line (`Lost lock
 race on token [...]`, visible once the sherdlock package's logger is at debug level) and in the
-[`tokendiag locks`](../../cmd/tokendiag/README.md) command.
+[`tokendiag locks`](../../cmd/tokendiag/README.md) command. `lock_store_errors_total` (also
+#2395) counts the sibling case: a TryLock/TryLockBatch failure that is *not* a lock conflict (does
+not wrap `driver.ErrTokenAlreadyLocked`) — a genuine store error such as a connection failure or
+timeout. To the caller both currently surface identically as retried, eventually-locked-funds
+contention, so this counter is what distinguishes "the store is unhealthy" from "tokens are just
+contended" without changing that retry behavior.
+
+`stale_candidates_total` (also #2395) counts the third case, which is neither: a candidate
+dropped because the token was no longer spendable by the time the lock was attempted
+(`driver.ErrTokenNotSpendable`). The eager fetcher serves candidates from a snapshot of the
+token store, so a token spent after that snapshot was taken is still offered until the cache
+refreshes; the lock is conditional on the token still being spendable, so such a candidate is
+rejected rather than handed to a caller that could not load it. A non-zero rate here therefore
+means the cache's freshness interval is long relative to how fast the wallet is spending, not
+that tokens are contended or that the store is unhealthy.
+
+**This counter only reports the single-token lock path.** A batch-capable backend — Postgres,
+via `LockBatch`, currently the only implementation — claims a whole window of candidates in one
+statement and answers with just the tokens it won, so a stale candidate is indistinguishable
+there from a token another claimant already holds and is counted under `lock_conflicts_total`
+instead. On such a deployment `stale_candidates_total` stays at zero *even during a
+stale-candidate episode*; the symptom to read is `lock_conflicts_total` rising without a
+corresponding rise in real contention (e.g. with `distinct_tokens_attempted` flat and no
+competing senders). Correctness is unaffected on either path — a token the store refuses is
+never handed to a caller — the difference is only in what is reported and in how quickly the
+selector's candidate cache learns it is behind the store.
+
+For `StubbornSelector`, both `selection_immediate_retries` and `distinct_tokens_attempted` are
+observed once per outer `Select()` call, aggregated over every internal backoff-retry attempt it
+makes — not once per attempt. They aggregate differently: `selection_immediate_retries` counts
+events and so sums the per-attempt counts, while `distinct_tokens_attempted` counts distinct
+tokens and so unions them, meaning a token contended across several attempts is counted once.
 
 | Metric | Type | Labels | Description |
 |---|---|---|---|
@@ -194,6 +225,8 @@ race on token [...]`, visible once the sherdlock package's logger is at debug le
 | `panurus_services_selector_sherdlock_selection_immediate_retries` | histogram | — | Distribution of immediate retry counts per token selection call |
 | `panurus_services_selector_sherdlock_lock_conflicts_total` | counter | — | Total number of lost lock races (a token was already locked by another process) |
 | `panurus_services_selector_sherdlock_distinct_tokens_attempted` | histogram | — | Distribution of the number of distinct tokens a lock was attempted on (won, lost, or rate-limited) per token selection call |
+| `panurus_services_selector_sherdlock_lock_store_errors_total` | counter | — | Total number of TryLock/TryLockBatch failures that are not lock conflicts (a genuine store error) |
+| `panurus_services_selector_sherdlock_stale_candidates_total` | counter | — | Total number of candidate tokens dropped because they were no longer spendable when the lock was attempted (single-token lock path only — see above) |
 
 Source: `token/services/selector/sherdlock/metrics.go`.
 

docs/development/sql-query-dsl.md

@@ -15,7 +15,7 @@ adding a store method or a new condition, not at application authors.
 | :--- | :--- |
 | `query` | Entry points: `Select()`, `Insert()`, `Update()`, `Delete()`, `Table()`. |
 | `query/common` | The `Builder` that accumulates SQL text plus bound parameters, and the `Serializable` / `Condition` / `CondInterpreter` contracts. |
-| `query/cond` | Condition constructors: `Eq`, `Cmp`, `In`, `InTuple`, `And`, `Or`, `Exists`, `BetweenTimestamps`, … |
+| `query/cond` | Condition constructors: `Eq`, `Cmp`, `In`, `InTuple`, `And`, `Or`, `Exists`, `NotExists`, `BetweenTimestamps`, … |
 | `query/select`, `query/insert`, `query/update`, `query/delete` | Per-statement builders. |
 | `query/pagination` | Pagination strategies and the interpreter that turns them into `LIMIT`/`OFFSET`/`WHERE` clauses. |
 

docs/services/finality.md

@@ -233,8 +233,13 @@ never causes or implies the other leg's transition.
    (`utils.RetryRunner`, `MaxRetry = 3`, one-second base backoff). Any error from `runOnStatus` —
    including a storage hiccup unrelated to the verdict itself — triggers a retry; only after all 3
    attempts fail does the listener call `OnError`, which bumps the `RetryExhausted` metric and logs,
-   leaving the transaction `Pending` for the recovery sweep (§5) to pick up later. `OnStatus` also
-   records the total wall-clock time (including retries) in the `OnStatusDuration` histogram.
+   leaving the transaction `Pending` for the recovery sweep (§5) to pick up later. When the retries are
+   exhausted, `OnStatus` also releases the transaction's selection locks exactly once — this is a
+   terminal give-up for the notification, whatever failed inside `runOnStatus`, so leaving the locks for
+   the lease-expiry sweep would reopen the contention window of
+   [#2395](https://github.com/LFDT-Panurus/panurus/issues/2395). `Unlock` is idempotent, and a later
+   selection attempt simply re-acquires what it needs. `OnStatus` also records the total wall-clock time
+   (including retries) in the `OnStatusDuration` histogram.
 
    Inside `runOnStatus`:
    - `network.Valid` → the token request to hash-check is fetched from `tokens.Service.GetCachedTokenRequest`
@@ -245,9 +250,14 @@ never causes or implies the other leg's transition.
      proceed to `Commit` (§4). **Mismatch** → `Deleted` + `HashMismatches` metric (§1 — this is folded
      into ordinary `Deleted`, not a distinct status, in the current implementation).
    - `network.Invalid` → `Deleted` directly.
-   - Anything else (`Busy`/`Unknown`) returns an error at this layer and is retried per the paragraph
-     above — the recovery handler (§5), by contrast, treats `Busy`/`Unknown` as an expected transient
-     state and simply releases its claim for the next sweep rather than erroring.
+   - `network.Busy`/`network.Unknown` → the transaction is not yet finalized. This is an expected
+     transient state, so `runOnStatus` logs at Debug and returns `nil` without touching the stores and
+     **without releasing the transaction's selection locks** — the transaction is still in flight, and
+     dropping its locks would let a concurrent `Select` re-offer the same tokens. This matches the
+     recovery handler's treatment of the same two statuses (§5).
+   - Any other, genuinely unrecognized status code returns an error at this layer and is retried per
+     the paragraph above. Retrying can never reclassify it, so the retries are exhausted and
+     `OnStatus`'s give-up branch releases the selection locks once (see below).
    - A verdict that resolves to `Deleted` increments `DeletedTransactions`; one that reaches `Commit`
      increments `ConfirmedTransactions` (both counted once `runOnStatus` returns successfully, not per
      retry attempt).
@@ -330,7 +340,7 @@ with no active listener anywhere. `TTXRecoveryHandler.Recover(ctx, txID)`
 ([`token/services/ttx/finality/recovery.go`](../../token/services/ttx/finality/recovery.go)) covers this
 — instead of waiting for a push event, it calls `Network.GetTransactionStatus(ctx, namespace, txID)`
 directly and runs the same decision logic as `runOnStatus` (`applyFinalityLogic`, sharing
-`checkTokenRequest` and `Commit`). Unlike the live listener path, `Busy`/`Unknown` here is treated as an
+`checkTokenRequest` and `Commit`). As on the live listener path, `Busy`/`Unknown` here is treated as an
 expected transient state, not an error: the handler simply returns `nil` without touching the status,
 releasing its claim so the periodic sweep in
 [Transaction Recovery Service](./storage/recovery.md) picks the transaction up again on its next pass
@@ -419,7 +429,7 @@ observable behavior at the `finalityView.Call` boundary (§3 step 10) is unchang
 | FabricX: pending waiter exceeds `pendingTTL` before a terminal status is polled | still `Pending`; the poller drops its own bookkeeping, no error surfaced there | the caller's own `finalityView` timeout (§6.3) surfaces this to the application; the recovery sweep also covers it |
 | Broadcast never reached the ordering service | permanently `Pending` (ledger never sees it) | recovery sweep marks it `Orphan` after its grace period, see [Transaction Recovery Service](./storage/recovery.md) |
 | Duplicate finality notification for the same tx | idempotent | `TransactionExists`-style guard in `AppendValid` |
-| `runOnStatus` errors 3 times in a row (e.g. a transient storage failure while writing `Confirmed`/`Deleted`) | still `Pending`; `RetryExhausted` metric incremented, `OnError` logs | recovery sweep (§5) re-derives status directly from the ledger on its own schedule — no automatic re-registration of this listener |
+| `runOnStatus` errors 3 times in a row (e.g. a transient storage failure while writing `Confirmed`/`Deleted`) | still `Pending`; `RetryExhausted` metric incremented, `OnError` logs, selection locks released once | recovery sweep (§5) re-derives status directly from the ledger on its own schedule — no automatic re-registration of this listener |
 | Recovery sweep disabled | any of the above `Pending`-stuck cases | none — only the live listener path (and its backend-specific fallback) remains; see [Transaction Recovery Service](./storage/recovery.md) for the enable/disable key |
 
 ## Related documents