fix(auditdb): correct locker reconciliation, classification and cleanup

Review follow-up on the #2040 fix. Seven defects, each with a regression
test verified to fail without the corresponding change.

In-memory locker:

- The idempotent-re-acquire reconciliation was a read-modify-write over two
  lock-free sync.Maps. Concurrent callers narrowing an anchor's ID set all
  read the same stale record, all concluded the same ID was stale, and all
  released its permit; x/sync/semaphore panics on over-release, so the
  previous change traded the type-assertion panic it fixed for a new one
  ("semaphore: released more than held"). Reconciliation now runs under a
  per-anchor lock. A single lock for the whole Locker would deadlock, 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 one entry per audited transaction.
- An empty re-acquire recorded the empty set, releasing the locks the anchor
  already held while returning nil: the caller kept believing it held them
  and another anchor could take them immediately. Acquiring nothing now
  leaves the anchor untouched, as the distributed locker already did.
- ErrLockContention was attached to every failed acquisition, so cancelling
  a request on a completely free ID was reported and counted as a lock
  conflict. The permit is now tried before blocking on it, so the sentinels
  are attached only when the ID was genuinely held.

Postgres locker:

- The outcome was classified by which context expired first. acquireDeadline
  defaults to a minute, so a request-scoped caller context is nearly always
  shorter, and a caller-driven timeout returned a bare
  context.DeadlineExceeded with no sentinel — this backend hardly ever
  reported contention in production while the in-memory one always did.
  Classification is now based on whether an attempt actually lost a race.
- A query killed by the acquire deadline carries context.DeadlineExceeded,
  so a database outage was relabelled as contention with the original error
  discarded, and isRetriableLockError then refused to retry it. The
  underlying error is always joined in, and ErrLockAcquireTimeout is
  attached only when the waiting budget is genuinely spent.
- The cleanup DELETE ran on a context that could neither be cancelled nor
  time out, so it could block indefinitely on a saturated connection pool
  or a conflicting row lock — leaving AcquireLocks hanging past the deadline
  it promises to honour and leaking the goroutine on shutdown. releaseAnchor
  now detaches and bounds, which also stops ReleaseLocks silently skipping
  the delete on an already-cancelled context and stranding leases until TTL.
- A failed re-acquire released the anchor unconditionally, deleting the
  lease rows of the live session it was re-acquiring for while leaving that
  session's record and heartbeat running; the next renewal reported the
  leases lost and the caller's next legitimate Append failed its pre-write
  assertion. Cleanup runs only when the anchor has no live session.

The conformance suite gave the Postgres locker a 300ms acquire deadline,
shorter than its own wait contexts, which masked the misclassification
above; it now uses a production-shaped deadline.

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

Author: AkramBitar

docs/services/auditor.md

@@ -70,6 +70,23 @@ Both backends implement the same contract, defined on the `Locker` interface, be
 
 **`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)):

plan.md

@@ -45,6 +45,54 @@ Fix the three findings in issue #2040 in `token/services/storage/auditdb/locker`
    `auditor.lock` and `auditor.locker`).
 8. [x] **Done** — `make unit-tests`, `make unit-tests-race` on the touched packages,
    `make lint-auto-fix`, `make checks`.
+9. [x] **Done** — Review round: fixed seven defects found in steps 3–5, each with a test that
+   fails without the fix (see "Review findings" below).
+
+## Review findings addressed
+
+The first pass fixed the three findings in #2040 but introduced or left seven defects of its
+own. All seven are fixed, and every one has a regression test verified to fail on the
+pre-review code.
+
+1. **New panic in the in-memory locker (critical).** The idempotent-re-acquire reconciliation
+   was a read-modify-write over two lock-free `sync.Map`s: concurrent callers narrowing an
+   anchor's ID set all read the same stale record, all concluded the same ID was stale, and
+   all released its permit. `golang.org/x/sync/semaphore` panics on over-release, so this
+   traded the type-assertion panic being fixed for a new one. Reproduced as
+   `panic: semaphore: released more than held`. Fixed with per-anchor locking (`anchorState`),
+   which makes the reconciliation atomic; a single lock for the whole `Locker` would deadlock,
+   since `AcquireLocks` blocks on permits that another anchor's release must hand over.
+   Anchor states are reference-counted and evicted so the map does not grow per transaction.
+2. **Empty re-acquire released the anchor's locks (high).** No early return for an empty set,
+   so `AcquireLocks(ctx, "a")` after `AcquireLocks(ctx, "a", "alice")` recorded the empty set,
+   released alice and returned `nil`. Exclusivity was gone with no error anywhere, and it was a
+   fresh divergence — the Postgres locker returns early.
+3. **Postgres almost never reported contention (high).** The outcome was classified by which
+   context expired first. `acquireDeadline` defaults to 1 minute, so a request-scoped caller
+   context is nearly always shorter, and a caller-driven timeout returned a bare
+   `context.DeadlineExceeded` with no sentinel. Now classified by whether an attempt actually
+   lost a race. `conformance_test.go` had masked this by giving the Postgres locker a 300 ms
+   deadline — shorter than the test's own wait — so the conformance locker now uses a
+   production-shaped 30 s deadline instead.
+4. **Unbounded cleanup context (medium).** `context.WithoutCancel(ctx)` has no deadline, so the
+   cleanup `DELETE` could block indefinitely on a saturated pool or a conflicting row lock —
+   leaving `AcquireLocks` hanging past the deadline it promises and leaking the goroutine on
+   shutdown. `releaseAnchor` now detaches *and* bounds (`releaseTimeout`), which also fixes
+   `ReleaseLocks` silently skipping the delete on an already-cancelled context.
+5. **Database failures relabelled as contention (medium).** A query killed by `acquireCtx` has
+   `context.DeadlineExceeded` in its chain, so an outage was reported as `ErrLockContention`
+   with the original error discarded — and `isRetriableLockError` then refused to retry it. The
+   underlying error is now always joined in, and `ErrLockAcquireTimeout` is attached only when
+   the waiting budget is genuinely spent.
+6. **Cancellation reported as contention (low).** `acquireError` attached `ErrLockContention`
+   unconditionally, so a shutdown cancellation on a completely free ID came back as a lock
+   conflict — the mirror image of finding 3's divergence. The sentinels are now attached only
+   when the ID was actually held, established by trying the permit before blocking on it.
+7. **Failed re-acquire tore down a live session (low).** The failure path released the anchor
+   unconditionally, deleting the lease rows of the live session it was re-acquiring for while
+   leaving that session's record and heartbeat running; the next renewal reported the leases
+   lost and the caller's next legitimate `Append` failed its pre-write assertion. Cleanup now
+   runs only when the anchor has no live session.
 
 ## Notes & Decisions
 

token/services/storage/auditdb/locker/conformance_test.go

@@ -68,11 +68,18 @@ func newPostgresLocker(t *testing.T) locker.Locker {
 	_, _ = db.Exec("DROP TABLE IF EXISTS " + table)
 	t.Cleanup(func() { _, _ = db.Exec("DROP TABLE IF EXISTS " + table) })
 
+	// AcquireDeadline is deliberately far longer than any context the cases below
+	// pass in, because that is the production shape: it defaults to a minute, so a
+	// request-scoped caller context is nearly always the shorter of the two. A
+	// deadline shorter than the caller's would let this backend pass the contention
+	// case for the wrong reason — the failure would be attributed to a budget this
+	// locker had spent itself, hiding that a caller-driven timeout reported no
+	// sentinel at all.
 	l, err := lockerpostgres.New(db, table, lockerpostgres.Config{
 		TTL:             30 * time.Second,
 		Heartbeat:       10 * time.Second,
 		AcquireBackoff:  10 * time.Millisecond,
-		AcquireDeadline: 300 * time.Millisecond,
+		AcquireDeadline: 30 * time.Second,
 		Owner:           "conformance-owner",
 	}, stubReplicaID{id: "conformance-owner"})
 	require.NoError(t, err)
@@ -200,6 +207,13 @@ func TestConformance_ReacquireSameAnchorIsIdempotent(t *testing.T) {
 // in a way callers can classify. auditor.Service inspects these sentinels to
 // decide whether retrying can help, so a backend that reports contention as a
 // bare context error is not interchangeable with one that does not.
+//
+// The caller's context here is much shorter than the Postgres backend's
+// AcquireDeadline, which is the production shape and the case that used to be
+// misclassified: that backend decided what to report from whichever context
+// expired first, so a caller-driven timeout returned a bare
+// context.DeadlineExceeded with no sentinel while the in-memory locker reported
+// contention for the identical situation.
 func TestConformance_ContentionIsReported(t *testing.T) {
 	for _, b := range backends() {
 		t.Run(b.name, func(t *testing.T) {
@@ -208,14 +222,72 @@ func TestConformance_ContentionIsReported(t *testing.T) {
 			require.NoError(t, l.AcquireLocks(ctx, "holder", "alice"))
 			t.Cleanup(func() { l.ReleaseLocks(ctx, "holder") })
 
-			waitCtx, cancel := context.WithTimeout(ctx, time.Second)
+			waitCtx, cancel := context.WithTimeout(ctx, 400*time.Millisecond)
 			defer cancel()
 			err := l.AcquireLocks(waitCtx, "waiter", "alice")
 
 			require.Error(t, err, "a lock held by another anchor must not be acquirable")
 			require.ErrorIs(t, err, errs.ErrLockContention)
 			require.ErrorIs(t, err, errs.ErrLockAcquireTimeout,
 				"having spent the whole waiting budget must be distinguishable from plain contention")
+
+			// The holder keeps its lock: a failed acquisition takes nothing away.
+			require.NoError(t, l.AssertLocksHeld(ctx, "holder"))
+		})
+	}
+}
+
+// TestConformance_EmptyReacquireKeepsHeldLocks pairs with EmptyEnrollmentIDs:
+// acquiring nothing succeeds, and must also leave alone whatever the anchor
+// already holds. The in-memory locker used to record the empty set instead, which
+// released those locks while returning nil — the caller kept believing it held
+// them and another anchor could take them immediately, with no error anywhere.
+// The Postgres locker returns early, so this was a divergence too.
+func TestConformance_EmptyReacquireKeepsHeldLocks(t *testing.T) {
+	for _, b := range backends() {
+		t.Run(b.name, func(t *testing.T) {
+			l := b.new(t)
+			ctx := context.Background()
+
+			require.NoError(t, l.AcquireLocks(ctx, "anchor1", "alice"))
+			require.NoError(t, l.AcquireLocks(ctx, "anchor1"), "acquiring nothing must succeed")
+			require.NoError(t, l.AssertLocksHeld(ctx, "anchor1"), "the anchor still holds alice")
+
+			waitCtx, cancel := context.WithTimeout(ctx, 300*time.Millisecond)
+			defer cancel()
+			require.Error(t, l.AcquireLocks(waitCtx, "anchor2", "alice"),
+				"an empty re-acquisition must not hand alice to another anchor")
+
+			l.ReleaseLocks(ctx, "anchor1")
+			require.NoError(t, l.AcquireLocks(ctx, "anchor2", "alice"),
+				"releasing the anchor must still release alice")
+			l.ReleaseLocks(ctx, "anchor2")
+		})
+	}
+}
+
+// TestConformance_CancelledCallerIsNotContention is the mirror image of
+// ContentionIsReported: a failure with no other holder involved must not be
+// dressed up as a lock conflict. The in-memory locker attached ErrLockContention
+// to every failed acquisition, so graceful-shutdown cancellations were reported
+// and counted as conflicts, while Postgres returned a plain context error.
+func TestConformance_CancelledCallerIsNotContention(t *testing.T) {
+	for _, b := range backends() {
+		t.Run(b.name, func(t *testing.T) {
+			l := b.new(t)
+			cancelled, cancel := context.WithCancel(context.Background())
+			cancel()
+
+			err := l.AcquireLocks(cancelled, "anchor1", "alice")
+			require.Error(t, err, "a caller that has given up must not be granted locks")
+			require.ErrorIs(t, err, context.Canceled)
+			require.NotErrorIs(t, err, errs.ErrLockContention,
+				"nothing held alice, so the failure is the caller's own cancellation")
+
+			// The failed call retained nothing, so alice is still free.
+			ctx := context.Background()
+			require.NoError(t, l.AcquireLocks(ctx, "anchor2", "alice"))
+			l.ReleaseLocks(ctx, "anchor2")
 		})
 	}
 }