Skip to content

fix(auditdb): align locker semantics, split key namespaces, stop nesting retries - #2211

Open
AkramBitar wants to merge 1 commit into
mainfrom
fix-2040-auditdb-locker
Open

fix(auditdb): align locker semantics, split key namespaces, stop nesting retries#2211
AkramBitar wants to merge 1 commit into
mainfrom
fix-2040-auditdb-locker

Conversation

@AkramBitar

@AkramBitar AkramBitar commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

In one paragraph

The auditor briefly "locks" each person involved in a transaction, so that two auditors can never write the same person's records at the same time. There are two interchangeable implementations of that lock, chosen by configuration: memory for a single node, postgres for a cluster. They did not behave the same way, which meant the same auditor code was correct on one deployment and broken on the other. This PR makes them behave identically, and adds a test suite that runs the same expectations against both so they cannot drift apart again.

Fixes #2040

What was broken

  1. A transaction with nobody to lock could not be written at all (Postgres only). Asked afterwards "do you still hold your locks?", the Postgres locker answered "no, you lost them" when the truth was "you never took any". That failed two legitimate flows: a transaction whose inputs and outputs yield no enrollment IDs, and an auditor that validates and appends without calling Audit.
  2. The auditor could crash (memory only). Two unrelated kinds of key — transaction IDs and person IDs — shared a single map. A transaction ID that happened to equal a person ID made the code read one kind of value as the other and panic.
  3. One audit could block for ~10 minutes. The auditor retried lock acquisition 10 times around the Postgres locker's own 1-minute internal wait, so the two waiting loops multiplied — and each of those attempts polled the database on a fixed interval, identical on every replica, so contenders stayed in lockstep and kept colliding.

In short

Think of two security guards (memory and postgres) with the same job: "make sure nobody edits Alice's records at the same time." They each had their own rulebook, and the rulebooks disagreed in 3 ways — one would crash if Alice's ID matched a transaction ID, one would falsely say "you lost access" for empty transactions, and both would keep each other waiting 10x longer than needed. This PR gives them one shared rulebook and fixes all three inconsistencies.

What this PR changes

  • "No locks" is success, on both backends. AssertLocksHeld reports locks that were lost, not locks that were never taken.
  • The in-memory locker keeps its two key namespaces in separate maps, so the collision is impossible by construction rather than by convention. Re-acquiring under a live transaction is now a proper refresh instead of overwriting the record and leaking the locks it dropped.
  • One layer does the waiting. The Postgres locker waits out contention itself with jittered exponential backoff bounded by a single deadline, and the auditor stops retrying as soon as it sees ErrLockAcquireTimeout — the locker saying "I already spent the whole budget". Worst case for one audit is now about one acquireDeadline, not maxRetries × acquireDeadline, and a contended wait costs a few dozen round trips instead of hundreds.
  • The contract is written down on the Locker interface, and locker/conformance_test.go runs it against every backend. The interface had no docs at all, which is how the two implementations drifted in the first place.

Testing

  • make checks and make lint-auto-fix clean.
  • go test -race green on token/services/storage/auditdb/... and token/services/auditor/..., with the Postgres cases running against a real container (not skipped).
  • Regression value verified by running each new test against the code it targets: the concurrency test reproduces panic: semaphore: released more than held; the four new Postgres tests fail with the exact misclassifications and deletions described above; the conformance suite fails EmptyEnrollmentIDs on Postgres only and ContentionIsReported on both once the masking deadline is corrected.

@AkramBitar AkramBitar added this to the Q3/26 milestone Aug 12, 2026
@AkramBitar AkramBitar self-assigned this Aug 12, 2026
@AkramBitar
AkramBitar marked this pull request as draft August 12, 2026 15:26
@AkramBitar
AkramBitar force-pushed the fix-2040-auditdb-locker branch from 1f2d770 to 985d315 Compare August 13, 2026 10:33
@AkramBitar
AkramBitar marked this pull request as ready for review August 13, 2026 10:34
@AkramBitar
AkramBitar force-pushed the fix-2040-auditdb-locker branch from 985d315 to c720bd7 Compare August 13, 2026 10:37
@Effi-S
Effi-S force-pushed the fix-2040-auditdb-locker branch from c720bd7 to f69b34e Compare August 17, 2026 13:06
Comment thread token/services/storage/auditdb/locker/memory/memory.go
@AkramBitar

Copy link
Copy Markdown
Contributor Author

Pushed 130991712 — fixes for the review findings. Each has a test verified to fail without it.

  • Postgres narrowing leaked lease rows. Re-acquisition only ever inserted, so an EID dropped from a live anchor kept its row — AssertLocksHeld and the heartbeat count rows per anchor and require an exact match, so writes were rejected with "locks lost before write", the heartbeat died on its first tick, and after the TTL the EID became claimable while this replica still thought it held it. Now deleted on the success path.
  • In-memory unlockAnchor could strand permits forever. It sampled "anchor is empty" before dropping the anchor lock and acted on it after taking the map lock, so an anchor holding permits could be evicted and those permits never reached again. Now read under both locks. The probe test stranded an EID after ~688 rounds on the old code.
  • @Effi-S — yes, the deadlock is real, and permanent. dedup.AndSort only orders the EIDs of one call, so already-held ones sit outside that order: a←{alice}, b←{bob}, then both widening to {alice,bob} never returned, and ReleaseLocks couldn't break it because the anchor lock is held across the blocking acquire. A live anchor's set may now shrink or stay the same but never grow — widening returns ErrLockSetWidened, which makes the cycle unconstructible rather than merely bounded. Audit acquires once per anchor, so nothing needed widening.
  • In-memory locker had no waiting budget. A caller with no deadline blocked forever and no failure could ever carry ErrLockAcquireTimeout. It now bounds itself with acquireDeadline, defaulting to the same 1m as Postgres.
  • isRetriableLockError stopped retrying transient DB errors. A locker's own budget elapsing surfaces as a bare context.DeadlineExceeded, so reading "caller is gone" from the error ended the loop after one attempt. Now read from ctx.
  • AuditLockConflicts counted non-conflicts, contradicting the error table in this PR. Now gated on ErrLockContention.
  • store.go no longer claims lock ordering alone prevents deadlock — it doesn't for incremental acquisition.

Two new conformance cases cover the shrinking and rejected-widening halves of the refresh contract, plus one that deliberately passes no caller deadline — which is what hid the missing budget.

@AkramBitar
AkramBitar force-pushed the fix-2040-auditdb-locker branch from 1309917 to 37f1eaf Compare August 18, 2026 15:51
@AkramBitar

Copy link
Copy Markdown
Contributor Author

@Effi-S could you please have additional look at this PR?
Thanks a lot,

@AkramBitar
AkramBitar force-pushed the fix-2040-auditdb-locker branch from 37f1eaf to 5aced16 Compare August 18, 2026 16:22
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. The Locker
interface now documents the contract its implementations are held to — it had
none, which is how they drifted — and locker/conformance_test.go exercises
every expectation against both backends.

The three findings in #2040:

- AssertLocksHeld detects lost locks, not absent ones. An anchor holding
  nothing succeeds, so a request whose inputs and outputs yield no enrollment
  IDs can be appended under postgres too, as can an auditor that validates and
  appends without calling Audit (the dvp and nft views).
- The memory locker keeps enrollment-ID semaphores and per-anchor bookkeeping
  in separate maps. One shared sync.Map, keyed by unconstrained strings of
  unrelated provenance, let an anchor equal to an enrollment ID return the
  other namespace's value type and panic on the assertion.
- The auditor's retry no longer nests inside the locker's own waiting budget:
  ErrLockAcquireTimeout is final, so worst-case blocking for one audit is
  acquireDeadline rather than MaxRetries times it. The inner poll loop is
  exponential and jittered instead of a flat 100ms, which cuts round trips
  from hundreds to a few dozen and stops contending replicas retrying in
  lockstep.

And the defects that aligning them surfaced:

- A live anchor's EID set may shrink or stay the same, never grow. Deadlock
  freedom rests on taking shared IDs in one canonical order, and that order
  only covers the IDs of a single call, so an anchor that kept earlier locks
  while waiting for new ones held locks outside it: two anchors widening into
  each other's IDs waited on each other forever, and permanently, since the
  anchor's lock is held across the blocking acquire and so blocked the release
  that would have broken the cycle. Widening now fails with ErrLockSetWidened.
- Postgres releases the leases a narrowing re-acquisition drops. Its upsert
  only inserted, and both AssertLocksHeld and the heartbeat require an exact
  row count per anchor, so each leftover row rejected the next write, killed
  the heartbeat, and then expired into another replica's hands.
- unlockAnchor no longer decides eviction from an emptiness flag sampled
  before the anchor's lock was released, which let an anchor still holding
  permits be dropped and those permits be stranded for the process lifetime.
- Each backend bounds its own waiting, so a caller that passes no deadline
  still gets an answer and a spent budget is reported as
  ErrLockAcquireTimeout.
- Failure classification is based on whether an attempt actually lost a race
  for an ID, not on which context expired first. The underlying error is
  joined in, so a database outage is reported as itself; a caller's own
  cancellation on a free ID is not a conflict; a locker's own expired budget
  is retriable while the caller's context is live; and only ErrLockContention
  counts towards auditor_audit_lock_conflicts_total.
- releaseAnchor detaches from the caller's context so a deferred release on an
  already-cancelled context still runs, and bounds itself so a stuck DELETE
  cannot outlive the deadline AcquireLocks promises. A failed re-acquisition
  no longer releases an anchor that already holds a live session.

Fixes #2040

Signed-off-by: AkramBitar <akram@il.ibm.com>
@AkramBitar
AkramBitar force-pushed the fix-2040-auditdb-locker branch from 5aced16 to e7c992b Compare August 18, 2026 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

auditdb/locker: misc medium/low findings (empty-lock semantics, key-namespace collision, retry storms)

2 participants