Retry registration serialization failures instead of misreporting them as duplicates

[devondragon]

Problem

Concurrent registrations of different emails can deadlock on index gap locks under the SERIALIZABLE registration transaction (MariaDB error 1213, Postgres serialization_failure). UserService.persistNewUserAccount translated any ConcurrencyFailureException into UserAlreadyExistException, so the deadlock victim was shown the anti-enumeration "Thank you for registering!" page while no account was created and no verification email sent — a silently lost registration.

Found during 5.3.0 release validation via the demo app's concurrent Playwright suite (registration.spec.ts flaking with 1213-40001: Deadlock immediately followed by User already exists for a fresh unique email). Reproduced on released 5.2.0 — affects all prior versions.

Fix

• persistNewUserAccount now translates only DataIntegrityViolationException (a true duplicate → 409) and lets serialization failures propagate.
• registerNewUserAccount retries the SERIALIZABLE write (5 attempts, growing jittered backoff). Each attempt persists a fresh entity copy — a rolled-back attempt leaves the original instance with a generated id and Hibernate collection state that would fail the retry — and re-runs the emailExists pre-check in a fresh transaction, so a genuine same-email race still yields the 409/anti-enumeration response.
• Exhausted retries surface the ConcurrencyFailureException (500) instead of a false success.
• SERIALIZABLE isolation itself is unchanged (preserved design decision).

The passwordless path is untouched: a deadlock there already surfaces as a 500 (honest, just not retried); it has no misreport bug.

Testing

• Unit: transient-failure retry succeeds; duplicate-found-on-retry still throws UserAlreadyExistException; exhausted retries propagate ConcurrencyFailureException (all watched fail first).
• Testcontainers (MariaDB + PostgreSQL): new test racing 6 distinct-email registrations, 3 repetitions each — failed against the old code and caught two entity-state bugs in earlier versions of the retry (id reuse → ObjectOptimisticLockingFailureException; collection state → orphan-delete error). Existing same-email race tests unchanged and passing.
• Full build: 1161 tests, 0 failures.

[claude]

Review

Solid fix for a real bug, and the root-cause analysis in the PR description is convincing (SERIALIZABLE + index gap locks deadlocking across unrelated rows is a well-known MariaDB/Postgres phenomenon, and translating any ConcurrencyFailureException to a 409 was clearly wrong). A few notes:

Correctness

• copyForInsert mirrors exactly the fields registerNewUserAccount sets before the first persist attempt (firstName, lastName, password, email, enabled) — I checked registerNewUserAccount (UserService.java:343-353) and there's nothing else set on user before the persist call, so the fresh-copy approach doesn't silently drop data on retry. Good catch on the original bug about reusing a rolled-back entity's Hibernate collection/id state.
• The retry correctly re-runs emailExists inside each attempt (via the proxied persistNewUserAccount), so a genuine same-email race still surfaces UserAlreadyExistException even after a retry — validated by shouldThrowUserAlreadyExistWhenRetryFindsConcurrentDuplicate.
• persistWithSerializationRetry is deliberately not transactional and is invoked directly (not through self), so the Thread.sleep backoff between attempts never holds a DB connection — consistent with the existing Propagation.NOT_SUPPORTED design on registerNewUserAccount.
• Backoff jitter: ThreadLocalRandom.current().nextLong(REGISTRATION_RETRY_BASE_DELAY_MS * attempt) is safe (bound is always > 0 since attempt >= 1).

Minor: CHANGELOG/code mismatch

CHANGELOG.md says retries happen "up to 3 attempts," but REGISTRATION_SERIALIZATION_ATTEMPTS = 5 in UserService.java, and the exhaustion test (shouldPropagateConcurrencyFailureWhenRetriesExhausted) asserts save is called 5 times. Worth aligning the changelog wording with the actual constant before merge.

Minor: style nits (per CLAUDE.md conventions)

• java.util.concurrent.ThreadLocalRandom.current() is used fully-qualified inline in persistWithSerializationRetry instead of via an import — inconsistent with the "alphabetical imports, no wildcards" convention (implies proper imports over inline FQNs).
• The new UserServiceTest cases use org.mockito.Mockito.times(...) inline rather than the import static org.mockito.Mockito.times; style already used elsewhere in the same file.

Design considerations (not blockers)

• Worst-case added latency on the request thread when contention is real: with 5 attempts and growing jittered backoff (25ms·attempt + jitter), a fully-exhausted retry sequence adds roughly several hundred ms of Thread.sleep before surfacing the 500. That's a reasonable trade for correctness, and no connection is held during the sleep, but it's worth confirming the demo app's Playwright suite/timeouts account for this worst case.
• log.warn fires on every failed attempt including the last (exhaustion) one — a distinct final log.error/message on total exhaustion could make prod alerting easier to distinguish from "retried and recovered," though this is a nice-to-have, not a bug.

Test coverage

Good coverage: transient-failure-then-success, duplicate-found-on-retry, retry-exhaustion (unit), plus a new Testcontainers test racing 6 distinct emails across MariaDB/Postgres that reportedly caught two real entity-state bugs during development. The unit tests correctly verify save invocation counts, which is the right way to pin down retry behavior without relying on timing.

Nice fix overall — the design commentary in the Javadoc is unusually thorough and will help future maintainers avoid regressing this.

[Copilot]

Pull request overview

This PR fixes a long-standing concurrency bug in the library’s SERIALIZABLE registration flow where serialization failures/deadlocks (often between different emails) were incorrectly translated into “user already exists”, causing silently lost registrations. The change adjusts exception translation and introduces a controlled retry loop so transient serialization failures succeed while true duplicates still produce the anti-enumeration 409 response.

Changes:

• Stop translating ConcurrencyFailureException to UserAlreadyExistException; only translate true duplicates (DataIntegrityViolationException).
• Add a SERIALIZABLE persist retry loop with jittered backoff and fresh entity instances per attempt.
• Expand unit + Testcontainers concurrency tests to cover transient failures, retry-found duplicates, and exhausted retries.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
src/main/java/com/digitalsanctuary/spring/user/service/UserService.java	Adds serialization retry wrapper and narrows duplicate translation to integrity violations only.
src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java	Adds unit tests validating retry success, duplicate detection on retry, and exhaustion behavior.
src/test/java/com/digitalsanctuary/spring/user/service/AbstractConcurrentRegistrationTest.java	Adds real-DB concurrent distinct-email registration test to ensure retries prevent lost registrations.
CHANGELOG.md	Documents the fixed misreporting/lost-registration behavior (needs attempt count alignment).

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[claude]

Review: Retry registration serialization failures instead of misreporting them as duplicates

Solid fix for a real bug — good root-cause diagnosis (deadlock victim swallowed as a false "already exists"), and the reproduction story via Testcontainers racing 6 distinct emails is convincing evidence the fix works. A few things worth a look before merge:

Bugs / inconsistencies

• CHANGELOG says "up to 3 attempts", code says 5. REGISTRATION_SERIALIZATION_ATTEMPTS = 5 in UserService.java:212 (and the exhausted-retry unit test asserts times(5)), but the new CHANGELOG.md entry reads "retried in a fresh transaction (up to 3 attempts)". Minor, but it's user-facing documentation of retry behavior and will mislead anyone tuning timeouts/expectations around it.

Design considerations (not blockers)

• Hardcoded retry tuning. REGISTRATION_SERIALIZATION_ATTEMPTS and REGISTRATION_RETRY_BASE_DELAY_MS are private constants rather than user.* properties. Given the library's convention of exposing nearly everything else (lockout thresholds, cron schedules, session-warn thresholds) via configuration, it might be worth exposing these too — different consumers will have different tolerances for registration latency vs. DB deadlock frequency. Not required for correctness, just a consistency nit with the rest of the config surface described in CLAUDE.md.
• Blocking Thread.sleep in the retry backoff (UserService.java:402) ties up the calling thread (a Tomcat worker, in the typical case) for up to ~4 backoff windows. Since registerNewUserAccount already runs with Propagation.NOT_SUPPORTED (no DB connection held during the sleep), this is a reasonable tradeoff and the worst case is bounded (roughly a few hundred ms with the current constants), but it's worth a one-line callout if there's ever a move to a reactive/virtual-thread stack where blocking sleeps in request threads are more of a concern.
• ThreadLocalRandom referenced fully-qualified inline (UserService.java:401) instead of a top-of-file import. CLAUDE.md's style guide calls for alphabetical imports/no wildcards; a normal import would be more consistent with the rest of the file's style even though this isn't a wildcard import.

Correctness spot-checks (all looked right)

• copyForInsert only copies firstName, lastName, password, email, enabled — exactly the fields registerNewUserAccount sets on the prototype before calling into persistence. registrationDate/lastActivityDate are handled by JPA auditing on insert, provider defaults to LOCAL, and roles is populated inside persistNewUserAccount itself — so nothing is silently dropped on retry.
• The split between DataIntegrityViolationException (translated to UserAlreadyExistException) and ConcurrencyFailureException (propagated to the retry) is exactly right for distinguishing "real duplicate" from "deadlock, possibly unrelated email" — and the retry's fresh emailExists pre-check correctly still catches a genuine same-email race after a failed attempt.
• persistNewUserAccount is only called from this one path in UserAPI — no other caller depends on the old blanket-translate-to-409 behavior for ConcurrencyFailureException.
• The passwordless registration path is correctly left untouched per the PR description (deadlocks there already surface honestly as a 500).

Test coverage

Good breadth: unit tests cover transient-failure-then-succeed, duplicate-found-on-retry, and exhausted-retries-propagate, and the new Testcontainers test (shouldRegisterEveryUserWhenThreadsRaceDifferentEmails, 6 threads × 3 repetitions against both MariaDB and Postgres) is a strong regression guard for the actual bug — nice that it's stated to have caught two more entity-state bugs (id reuse, orphan-delete) during development.

One smaller note on the test-infra changes (UserApiTest.deleteTestUser, CaptchaProtectionIntegrationTest.deleteTestUser): the fixed Thread.sleep(100) + 3-attempt retry loop for the FK race against the async RegistrationListener works, but the project already depends on Awaitility (per CLAUDE.md) specifically for this kind of "wait for an async side effect to settle" scenario — Awaitility.await().atMost(...).untilAsserted(...) around the delete would express the same intent without a hand-rolled retry/backoff loop duplicated across two test files.

Overall: correct fix for a genuinely nasty bug, well-tested, and the tradeoffs (bounded retry, blocking sleep, hardcoded constants) are reasonable defaults. The CHANGELOG attempt-count mismatch is the one thing I'd fix before merge.