fix(auditdb): align locker semantics across backends

The auditor takes short-lived locks on the enrollment IDs a request
touches, through a Locker chosen from configuration: memory for a single
replica, postgres for a cluster. The two were not interchangeable, so the
same auditor code was correct on one deployment and broken on the other.

Issue #2040 found three symptoms:

- Postgres returned ErrLockNotHeld for any anchor without a session, so
  AssertLocksHeld conflated "lost the locks I took" with "never took any".
  StoreService.Append calls it on every write, so a request whose inputs
  and outputs yield no enrollment IDs could never be appended — and nor
  could an auditor that validates and appends without calling Audit, as
  the dvp and nft views do.
- The in-memory locker kept enrollment-ID semaphores and per-anchor ID
  lists in one sync.Map. Both are keyed by unconstrained strings of
  unrelated provenance, so an anchor equal to an enrollment ID made a
  lookup return the other namespace's value type, and the unchecked type
  assertion on it panicked.
- auditor.Service retried AcquireLocks up to MaxRetries times around the
  Postgres locker's own AcquireDeadline wait, so the two loops multiplied:
  worst case ten minutes of blocking for a single audit, polled at a flat,
  un-jittered interval that kept contending replicas in lockstep.

The contract, and the fix:

- The Locker interface now states the contract its implementations are
  held to. It had no documentation at all, which is how they drifted.
  AssertLocksHeld detects lost locks, not absent ones. AcquireLocks is
  all-or-nothing, treats an empty set as a successful acquisition of
  nothing, and is idempotent under a live anchor. conformance_test.go runs
  the shared expectations against every backend.
- The in-memory locker splits the two key namespaces into separate maps,
  so the collision is impossible by construction rather than by
  convention. Reconciliation on re-acquisition runs under a per-anchor
  lock: it is a read-modify-write of the anchor's record, and done
  lock-free, two concurrent callers narrowing the set both release the
  same permit — which panics golang.org/x/sync/semaphore rather than being
  a no-op. One lock for the whole Locker would deadlock instead, since
  AcquireLocks blocks on permits another anchor's release must hand over.
  Anchor states are reference-counted and evicted, so the map does not
  grow by an entry per audited transaction.
- The Postgres locker does all the waiting itself, with jittered
  exponential backoff bounded by one derived context, and auditor.Service
  no longer retries an error carrying ErrLockAcquireTimeout. Worst case is
  now about one AcquireDeadline, at a few dozen round trips rather than
  hundreds. AcquireMaxBackoff (default 2s) caps the growth.
- Failure classification is based on whether an attempt actually lost a
  race for an ID, not on which context expired first. AcquireDeadline
  defaults to a minute, so a request-scoped caller context is nearly
  always the shorter of the two, and keying off it meant Postgres hardly
  ever reported contention in production while memory always did. The
  underlying error is always joined in, so a database outage is reported
  as itself instead of being relabelled as contention with its details
  discarded. Conversely, a caller's own cancellation on a free ID is no
  longer reported as a conflict.
- releaseAnchor detaches from the caller's context, so a deferred release
  on an already-cancelled context still runs instead of stranding leases
  until their TTL, and bounds itself so a stuck DELETE cannot block past
  the deadline AcquireLocks promises to honour. A failed re-acquisition no
  longer releases an anchor that already holds a live session, which used
  to delete that session's leases while its heartbeat kept running and
  fail the caller's next legitimate Append with "locks lost before write".

Fixes #2040

Signed-off-by: AkramBitar <akram@il.ibm.com>

Author: AkramBitar

docs/configuration.md

@@ -323,9 +323,16 @@ token:
           postgres:
             # ttl is the lease duration for each EID lock row.
             ttl: 30s
-            # acquireBackoff is the wait between retry attempts when a lock is contended.
+            # acquireBackoff is the initial wait between retry attempts when a lock
+            # is contended. Successive waits grow exponentially and are jittered,
+            # so this is the floor rather than a fixed poll interval.
             acquireBackoff: 100ms
-            # acquireDeadline is the total time allowed to acquire all EID locks.
+            # acquireMaxBackoff caps that growth. Raised to acquireBackoff if set
+            # below it.
+            acquireMaxBackoff: 2s
+            # acquireDeadline is the total time allowed to acquire all EID locks,
+            # and the whole budget for waiting out contention: auditor.lock does
+            # not retry an acquisition that already exhausted it.
             acquireDeadline: 1m
             # heartbeat is the interval at which held leases are renewed (~TTL/3).
             heartbeat: 10s
@@ -810,6 +817,14 @@ Default values:
 - **backoffMultiplier**: Factor by which the backoff delay increases after each retry (exponential growth)
 - **jitterFactor**: Randomization factor (0.0 to 1.0) added to backoff delays to prevent multiple auditors from retrying simultaneously (prevents thundering herd problem)
 
+**Relationship to `auditor.locker`:** this retry covers failures that another attempt
+might survive, such as a transient database error. It deliberately does **not** retry
+an acquisition that failed with `ErrLockAcquireTimeout`, because that means the locker
+already spent its own `acquireDeadline` waiting out the contention — retrying would
+spend it again. Waiting under contention is configured once, in
+[`auditor.locker`](#optional-tokentmsauditorlocker), so `maxRetries` here does not
+multiply `acquireDeadline` there.
+
 **Tuning Recommendations:**
 
 1. **For High-Contention Environments:**
@@ -924,6 +939,7 @@ token:
           postgres:
             ttl: 30s
             acquireBackoff: 100ms
+            acquireMaxBackoff: 2s
             acquireDeadline: 1m
             heartbeat: 10s
             owner:
@@ -933,8 +949,9 @@ Default values:
 
 - backend: `memory` (in-process mutex, single-replica only)
 - postgres.ttl: 30s
-- postgres.acquireBackoff: 100ms
-- postgres.acquireDeadline: 1m
+- postgres.acquireBackoff: 100ms (initial wait; grows exponentially, jittered)
+- postgres.acquireMaxBackoff: 2s (cap on that growth; raised to `acquireBackoff` if set below it)
+- postgres.acquireDeadline: 1m (the whole budget for waiting out contention)
 - postgres.heartbeat: 10s
 - postgres.owner: empty, defaults to the FSC node ID (`config.Provider.ID()`). Required
   when `backend: postgres` — if both this value and `fsc.id` are empty or blank, the

docs/services/auditor.md

@@ -83,6 +83,27 @@ When multiple auditor replicas share the same AuditDB (PostgreSQL), concurrent p
 
 The locker is injected into `auditdb.StoreService` at startup. Before appending audit records, the store calls `AssertLocksHeld` to verify leases are still valid.
 
+Both backends implement the same contract, defined on the `Locker` interface, because the backend is chosen from configuration — a behavioural difference between them would be a correctness difference between two deployments running the same code. The shared expectations are exercised against every backend by `locker/conformance_test.go`.
+
+**`AssertLocksHeld` detects lost locks, not absent ones.** It fails only when a lease this replica held has since expired or been taken over. An anchor that holds nothing succeeds — whether because the request yielded no enrollment IDs, or because the caller never locked anything for it. The latter is a supported flow: an auditor may call `Validate` (which takes no locks) and then `Append`, as the dvp and nft sample views do.
+
+**`AcquireLocks` is all-or-nothing, and never gives up ground.** A failed call holds none of the EIDs it had reached for, and leaves untouched whatever the anchor already held from an earlier successful call. Re-acquiring under a live anchor is a refresh: the EIDs it already holds are kept, new ones are added, and ones it no longer needs are released. Acquiring an *empty* set is a successful acquisition of nothing — it must not, and does not, release what the anchor is already holding.
+
+**Release always runs.** `ReleaseLocks` is idempotent, safe on an anchor that never acquired anything, and safe to `defer`. Its statement is deliberately detached from the caller's context, since the usual case is a deferred release on a context that is already cancelled and a skipped release would leave the EIDs locked against every replica until the lease TTL expired. It carries its own short internal deadline so a stuck release cannot outlive the call that issued it.
+
+### Error classification
+
+Callers act on the outcome of a failed acquisition, so both backends classify it the same way:
+
+| Outcome | Sentinels | Meaning |
+|---------|-----------|---------|
+| Another anchor holds an EID and the waiting budget ran out | `ErrLockContention` + `ErrLockAcquireTimeout` | A real conflict, already waited out in full; retrying only adds delay |
+| Another anchor holds an EID, but a transient failure or a cancelled caller ended the wait | `ErrLockContention` | A real conflict, not yet waited out; a later attempt may succeed |
+| The caller cancelled or ran out of time while nothing held the EIDs | neither (plain context error) | Not a conflict, and not counted as one |
+| The database failed | neither; the underlying error is preserved | An infrastructure fault, reported as itself rather than as contention |
+
+The distinction is drawn from whether an attempt actually lost a race for an EID — not from which context expired first. `acquireDeadline` defaults to a minute, so a request-scoped caller context is almost always the shorter of the two, and keying off it would mean the Postgres backend hardly ever reported contention in production.
+
 ### Configuration
 
 Configure under `token.tms.<name>.auditor.locker` (see [Configuration](../configuration.md#optional-tokentmsauditorlocker)):
@@ -96,8 +117,9 @@ token:
           backend: postgres   # use "memory" (default) for single replica
           postgres:
             ttl: 30s
-            acquireBackoff: 100ms
-            acquireDeadline: 1m
+            acquireBackoff: 100ms     # initial wait; grows exponentially, jittered
+            acquireMaxBackoff: 2s     # cap on that growth
+            acquireDeadline: 1m       # whole budget for waiting out contention
             heartbeat: 10s
             owner:            # required; defaults to the FSC node ID
 ```
@@ -113,6 +135,12 @@ The Postgres backend creates an `eid_leases` table (prefixed per TMS persistence
 
 **Lease ownership:** each row is keyed by EID and carries the holding replica (`owner`) plus the request it was taken for (`anchor`). An acquisition may only take over an existing row when the lease has expired, or when the row is the same replica's lease for the *same* anchor — a re-acquisition, which just refreshes the deadline. Two different anchors therefore never hold the same EID at once, including two concurrent audits on a single replica; the second one is contended and retried until `acquireDeadline`.
 
+### Waiting under contention
+
+`acquireDeadline` is the **entire** budget for waiting out a contended EID, and one layer does the waiting. Within it, the locker retries the atomic acquisition starting at `acquireBackoff`, doubling up to `acquireMaxBackoff`, with jitter on every sleep. The jitter matters as much as the growth: a fixed interval is identical on every replica, so contenders retry in lockstep and keep colliding, and at the defaults it also costs one round trip per interval for the whole deadline — hundreds per acquisition.
+
+`auditor.Service` wraps `AcquireLocks` in its own retry (`token.tms.<name>.auditor.lock`) for transient failures, but it does **not** retry an error carrying `ErrLockAcquireTimeout`: the locker reporting one means it already spent `acquireDeadline`, so calling it again would spend that deadline afresh rather than improve the odds. Worst-case blocking for one audit is therefore about `acquireDeadline`, not `maxRetries × acquireDeadline`.
+
 ### Replica owner identity
 
 Every lease row carries an `owner` column, and each replica scopes all of its lease

token/services/auditor/auditor.go

@@ -193,6 +193,13 @@ func (a *Service) Audit(ctx context.Context, tx Transaction) (*token.InputStream
 // acquireLocksWithRetry attempts to acquire locks with exponential backoff and randomized jitter
 // to prevent livelock conditions when multiple auditors compete for the same enrollment IDs.
 // This implements the mitigation strategy for deadlock/livelock prevention.
+//
+// The locker owns the waiting policy and bounds it itself, so this loop must not
+// re-run an attempt that already spent that budget: an error carrying
+// ErrLockAcquireTimeout is final here. Retrying it anyway multiplied the locker's
+// deadline by MaxRetries — worst case, ten minutes of blocking for a single audit
+// against the Postgres backend, on top of the round trips each of those attempts
+// spent polling. Context errors are final for the same reason: the caller is gone.
 func (a *Service) acquireLocksWithRetry(ctx context.Context, anchor string, eids []string) error {
 	// Create a retry runner with jitter support
 	retryRunner := utils.NewRetryRunnerWithJitter(
@@ -204,18 +211,34 @@ func (a *Service) acquireLocksWithRetry(ctx context.Context, anchor string, eids
 		a.lockConfig.JitterFactor,
 	)
 
-	// Use the retry runner to acquire locks
-	err := retryRunner.RunWithContext(ctx, func() error {
-		return a.auditDB.AcquireLocks(ctx, anchor, eids...)
-	})
+	// Use the retry runner to acquire locks, stopping early on errors that another
+	// attempt cannot improve on.
+	err := retryRunner.RunWithErrorsContext(ctx, func() (bool, error) {
+		err := a.auditDB.AcquireLocks(ctx, anchor, eids...)
+		if err == nil {
+			return true, nil
+		}
 
+		return !isRetriableLockError(err), err
+	})
 	if err != nil {
 		return errors.WithMessagef(err, "failed to acquire locks for anchor [%s]", anchor)
 	}
 
 	return nil
 }
 
+// isRetriableLockError reports whether re-running AcquireLocks stands a chance of
+// a different outcome. Most failures do — a contended lock may be free by now, a
+// database blip may have passed — but two do not: ErrLockAcquireTimeout means the
+// locker already spent its whole waiting budget, so an identical attempt would
+// just spend it again, and a context error means the caller is no longer waiting.
+func isRetriableLockError(err error) bool {
+	return !errors.Is(err, auditdb.ErrLockAcquireTimeout) &&
+		!errors.Is(err, context.Canceled) &&
+		!errors.Is(err, context.DeadlineExceeded)
+}
+
 // Append adds the passed transaction to the auditor database, reusing the
 // audit record computed by Audit, when available.
 // It also releases the locks acquired by Audit.

token/services/auditor/auditor_test.go

@@ -1081,6 +1081,32 @@ func TestService_AcquireLocksWithRetry_Failure_MaxRetriesExceeded(t *testing.T)
 	assert.Equal(t, 10, mockLocker.GetCallCount(), "Should retry max times (default is 10)")
 }
 
+// TestService_AcquireLocksWithRetry_NoRetryAfterLockerTimeout covers issue
+// #2040's third finding. A locker that reports ErrLockAcquireTimeout has already
+// spent its own acquisition deadline waiting out the contention, so repeating the
+// call spends that deadline again rather than giving the caller a fresh chance.
+// This loop used to retry it MaxRetries times regardless: nested inside the
+// Postgres locker's one-minute deadline that meant up to ten minutes of blocking
+// for a single audit, and thousands of database round trips.
+func TestService_AcquireLocksWithRetry_NoRetryAfterLockerTimeout(t *testing.T) {
+	mockLocker := newMockAuditLocker(func(ctx context.Context, anchor string, eIDs ...string) error {
+		return errors.Join(auditdb.ErrLockAcquireTimeout, auditdb.ErrLockContention)
+	})
+	svc := newTestServiceWithMockLocker(t, mockLocker)
+
+	_, _, err := svc.Audit(context.Background(), &auditmock.Transaction{
+		IDStub: func() string { return "tx-lock-timeout" },
+		RequestStub: func() *token.Request {
+			return token.NewRequest(newTestManagementService(t), token.RequestAnchor("tx-lock-timeout"))
+		},
+	})
+
+	require.Error(t, err)
+	require.ErrorIs(t, err, auditdb.ErrLockAcquireTimeout, "the locker's verdict must reach the caller intact")
+	assert.Equal(t, 1, mockLocker.GetCallCount(),
+		"a locker that already exhausted its acquire deadline must not be called again")
+}
+
 func TestService_AcquireLocksWithRetry_ContextCancelled_BeforeRetry(t *testing.T) {
 	// Mock locker that always fails
 	mockLocker := newMockAuditLocker(func(ctx context.Context, anchor string, eIDs ...string) error {