fix(recovery): persist audit recovery claims

[atharrva01]

Fixes the second half of #2143, the one that causes the duplicated work.

postgres.AuditTransactionStore only overrode WriteDB, GetSchema and CreateSchema, so it
inherited ClaimPendingTransactions and ReleaseRecoveryClaim from sqlcommon. Those are a
plain SELECT and a no-op, so no claim is ever persisted and every replica selects the same
pending audit transactions on every tick and processes all of them. The audit store now uses the
same atomic UPDATE ... RETURNING claim the owner store has had since it was introduced.

The SQL is identical for the two stores and only the requests table differs, so it moves into a
shared recoveryClaimStore that both hold as a field. It is a named field and not an embedded
one on purpose: sqlcommon.TransactionStore provides the same two methods, and embedding both at
the same depth makes the selectors ambiguous, which drops them from the method set and quietly
stops the store satisfying driver.AuditTransactionStore. Explicit forwarding keeps the override
visible where it is called.

No schema change. recovery_claimed_by, recovery_claim_expires_at and the two supporting
indexes already come from the shared sqlcommon schema that audit storage uses.

Left for a follow-up

The other half of #2143, leadership always being granted on the audit path, is not fixed here.
That needs a lock id derived per store, which is exactly what #2085 introduces, and doing it on
top of main would mean inventing a second scheme that competes with the one already in review.
Worth noting that both recovery managers currently read the same AdvisoryLockID from config, so
handing audit a real factory today would make owner and audit recovery contend on one lock and
starve each other.

This is still worth landing on its own. With the atomic claim in place, each pending audit
transaction goes to exactly one replica even while every replica believes it is the leader, so
the duplicated processing is gone. What remains is N cheap claim queries per tick instead of one.

Question

CleanupExpiredClaims is not exposed on the audit store. Nothing calls it on either path, and
the claim query already reclaims expired leases inline through recovery_claim_expires_at < NOW(),
so adding it looked like new dead code. Happy to add it for symmetry if you would rather have it.

Tests

Five new tests in audit_recovery_claim_test.go mirroring the owner ones: claim exclusivity
across two replicas, lease expiry, release making a row immediately available again, release
under the wrong owner being ignored, and one pinning that audit and owner claims stay in their
own requests tables. The existing owner tests pass unchanged, ageRequests just takes the claim
store now so both suites can share it.

[atharrva01]

@adecaro , @AkramBitar , let me know your thoughts on this , whenever you have chance , thanks :)

[AkramBitar]

@atharrva01,

Thanks for the fix. Based on my understanding the root cause is clear and the approach is correct. The audit store was inheriting a no-op claim path from sqlcommon, so every replica was picking up the same pending transactions on every sweep and processing them all. The atomic UPDATE ... RETURNING with a lease is the right solution, and the tests cover the important cases well.

One concern I'd like to understand better: what happens when processing takes longer than the lease duration?

Once a replica claims a transaction and starts processing it, if the work exceeds LeaseDuration, the lease expires and another replica can claim and process the same transaction, which is exactly the duplication this PR is trying to prevent, just now under a race condition rather than always.

Could you clarify:

1. Is the audit transaction processing guaranteed to be idempotent? If a transaction gets processed twice, is that safe?
2. Is there a lease renewal / heartbeat mechanism in place, or planned?
3. How is LeaseDuration configured and what is the expected worst-case processing time? Is there any guardrail to ensure the lease is long enough?

If the work is idempotent, this is acceptable as-is and worth a comment saying so. If it's not, the lease expiry window is a correctness gap that should be addressed before or shortly after this lands.

In addition, does is the last thing that we need to do for the issue #2143 cna we attach it to this PR and close one we close this PR?

[atharrva01]

Thanks @AkramBitar, fair thing to press on. Short version: the lease isn't what provides mutual exclusion here, leadership is.

runSweep (recovery/manager.go:198-215) takes the advisory lock, then claims and processes inside it, releasing only after workerWG.Wait(). So while one replica processes, the others get acquired=false and skip the tick. A lease expiring can't hand the row to anyone, because nobody else is claiming at that moment. The lease exists for the crash case: if the leader dies mid-sweep the lock auto-releases with the session, and the expired lease is what lets the next leader re-claim rows the dead replica never released.

Caveat I should have put in the PR description: that doesn't hold on the audit path yet. This PR leaves NewAuditTransactionStore on the nil-factory constructor, so AcquireRecoveryLeadership returns noopRecoveryLeadership{}, true, nil (sql/common/transactions.go:375-381) and every audit replica still thinks it's leader. That's the second half of #2143. It's still worth landing alone, because the atomic claim removes the duplication on its own: even with every replica sweeping, FOR UPDATE SKIP LOCKED gives each row to exactly one of them. Leadership then turns N sweeps into one.

1. Idempotent? Mostly, and I'd rather be specific than claim it wholesale. AppendValid short-circuits on TransactionExists (tokens/tokens.go:110-120) so nothing is re-appended or re-spent, and ReleaseRecoveryClaim is fenced on recovery_claimed_by = $owner, so a slow ex-owner can't clear someone else's claim. Not clean: metrics double-count, TransactionExists is check-then-act, and the Orphan SetStatus isn't owner-fenced. Happy to add this as a comment on the claim path if the summary looks right to you.

2. Renewal? None today. Precedent if we want one: auditdb/locker/postgres runs a heartbeatLoop renewing at ~TTL/3 (locker/postgres/postgres.go:118-138).

3. Config? Defaults 30s lease / 30s TTL / 5s scan / batch 100 / 4 workers. validateConfig only checks > 0; docs/configuration.md:782 advises 2x expected processing but nothing enforces it. Two real gaps worth naming: the leader's advisory-lock connection dropping (PG restart, pooler kill) releases the lock mid-processing, and separately m.ctx has no deadline while GetTransactionStatus is a network call, so a hung query stalls the sweep and with leadership blocks every replica's sweep. That second one looks worse than the lease window to me and is pre-existing. Glad to open an issue and take a per-tx timeout as a follow-up.

4. #2143 — not the last piece, so I'd rather it not auto-close. The leadership half is written and tested but sits on #2085, which changes the leader factory signature to bind the lock id at construction; doing it on main means inventing a scheme #2085 deletes. There's also a trap: giving audit the owner's lock id makes the two managers contend so one never sweeps, which is why it's derived per store. Preference is to link this as partial and let the leadership PR carry Fixes #2143. If you'd rather see one change, I can hold this and stack them.

[AkramBitar]

@atharrva01 thanks a lot for the response.

So this PR should go after PR #2085 (i.e., wait for this one to be merged first)?

[atharrva01]

yessssss @AkramBitar