Retry a PAM lease mint the provider could not serialize

[Hinton]

🎟️ Tracking

No ticket — a CI flake, first triaged on #8243 where it failed the mint on Postgres.

📔 Objective

AccessLeaseRepository.CreateFromApprovedRequestAsync (EF) enforces its per-cipher singleton with a predicate read inside a Serializable transaction:

var conflict = await dbContext.AccessLeases
    .AnyAsync(l => l.CipherId == cipherId.Value
        && l.Status == AccessLeaseStatus.Active
        && l.NotBefore <= now
        && l.NotAfter > now);

PostgreSQL records a predicate lock for that read, so the transaction becomes a candidate for abort whenever any other transaction inserts a lease before it commits — including one granting access to an unrelated cipher, and including one at a weaker isolation level, since only the reader has to be Serializable for SSI to find a read-write dependency. The abort escaped the repository as a raw Npgsql.PostgresException: 40001, which means a 500 for a caller whose activation was merely unlucky, and an intermittently red CI as soon as anything else seeds a lease concurrently.

What this changes

The single attempt is now retried on a provider serialization failure. Each attempt takes a fresh scope, context and transaction and re-reads the state its guard needs, so a losing caller reaches the same deterministic outcome that the SQL Server procedure's UPDLOCK/HOLDLOCK blocks for. Retries are bounded and anything outliving them propagates — on a path that grants access to Vault Data, an unresolved persistence failure must not be reported as a benign mint outcome.

This is the contract PostgreSQL specifies for SERIALIZABLE: applications at that level must be prepared to retry on serialization failure. The stale comment telling callers to treat the exception as a conflict is updated, since the repository now handles it.

Deliberately unchanged:

• The guard's semantics. Same isolation level, same predicate, same outcomes.
• The MSSQL path. The Dapper procedure blocks rather than aborting, and never flaked.
• The schema. A partial unique index on (CipherId) WHERE Status = Active can't replace the guard: the invariant is "Active and the window covers now", and an index predicate can't reference now.

Testing

CreateFromApprovedRequestAsync_ConcurrentSameCipherActivations_OneMintsAndTheOtherConflicts races two activations of one cipher on separate connections and asserts exactly one mint, one conflict, and one surviving lease. Without the retry it reproduces the escaping 40001 on 3 of 3 runs; with it, it passes on SqlServer, Postgres and Sqlite.

At suite level, --filter "FullyQualifiedName~IntegrationTest.Pam" on Postgres hit 40001 in 1 of 6 runs before the change and 0 of 8 after.

Not verified locally: MySQL (my local container's credentials are broken, so every MySql case errors on connect) — relying on CI for that provider.

📸 Screenshots

n/a

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the bounded retry added to the EF AccessLeaseRepository.CreateFromApprovedRequestAsync mint path and the new concurrent-activation integration test. The retry is correctly bounded — the exception filter's attempt < MaxMintAttempts guard lets the third failure propagate rather than loop — and each attempt takes a fresh scope, context and transaction, with the aborted transaction rolled back by await using disposal before the loop re-enters; the caller's lease is never mutated, so no state carries across attempts. IsSerializationFailure maps the right codes per provider (Npgsql 40001/40P01, MySQL 1213/1205, SQL Server 1205, SQLite SQLITE_BUSY/SQLITE_LOCKED) and stays narrow enough that genuine persistence failures still surface instead of being reported as a benign mint outcome. The documentation-placement defect raised on the previous review is resolved in 84b2fac96.

Code Review Details

• ❓ : AccessRequestRepository.CreateApprovedExtensionAsync reads the same kind of predicate (AnyAsync(r => r.ExtensionOfLeaseId == ...)) inside a Serializable transaction and still carries the now-superseded "callers should be prepared to treat such an exception as a conflict" guidance — was leaving that path unretried deliberate, given it is exercised by the same IntegrationTest.Pam suite?
  • src/Infrastructure.EntityFramework/Pam/Repositories/AccessRequestRepository.cs:342

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.88%. Comparing base (aebb9c4) to head (84b2fac).

Files with missing lines	Patch %	Lines
...ramework/Pam/Repositories/AccessLeaseRepository.cs	66.66%	4 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8253      +/-   ##
==========================================
- Coverage   68.88%   68.88%   -0.01%     
==========================================
  Files        2410     2410              
  Lines      104442   104463      +21     
  Branches     9457     9461       +4     
==========================================
+ Hits        71946    71959      +13     
- Misses      30115    30120       +5     
- Partials     2381     2384       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.