From 1d417d00a92ea3c07f541b9922f17634fd8d68b1 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Fri, 14 Aug 2026 22:36:33 -0600 Subject: [PATCH 1/2] fix: retry serialization failures during registration instead of misreporting them as duplicates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Concurrent registrations of DIFFERENT emails can deadlock on index gap locks under the SERIALIZABLE registration transaction (MariaDB 1213, Postgres serialization_failure). persistNewUserAccount translated any ConcurrencyFailureException into UserAlreadyExistException, so the deadlock victim's caller rendered the anti-enumeration success page while no account was created and no verification email sent — a silently lost registration. Found via the demo app's concurrent Playwright suite during 5.3.0 release validation; reproduced on 5.2.0. persistNewUserAccount now translates only DataIntegrityViolationException (a true duplicate) and lets serialization failures propagate to a new bounded retry in registerNewUserAccount (5 attempts, growing jittered backoff). Each retry persists a fresh entity copy — a rolled-back attempt leaves the original instance carrying a generated id and Hibernate collection state, which would otherwise fail the retry with optimistic-locking/orphan-delete errors — and re-runs the emailExists pre-check in a fresh transaction, so a genuine same-email race still returns the 409/anti-enumeration response. Exhausted retries surface the ConcurrencyFailureException (500) rather than a false success. SERIALIZABLE isolation itself is unchanged. Tests: unit coverage for transient-failure retry, duplicate-on-retry, and retry exhaustion; new Testcontainers test racing 6 distinct-email registrations on MariaDB and PostgreSQL (failed against the old code with the misreport, and caught both entity-state bugs in the retry). --- CHANGELOG.md | 1 + .../spring/user/service/UserService.java | 116 +++++++++++++++--- .../AbstractConcurrentRegistrationTest.java | 54 +++++++- .../spring/user/service/UserServiceTest.java | 48 +++++++- 4 files changed, 195 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e0526c7a..43828326 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ All notable changes to this project are documented here. This project follows [S - Remember-me enabled without a signing key, and `usePersistentTokens=true` without a `PersistentTokenRepository` bean, now log explicit warnings instead of silently skipping/downgrading. ### Fixed +- Concurrent registrations of **different** emails could deadlock under the SERIALIZABLE registration transaction (MariaDB error 1213) and the victim was misreported as "user already exists": the person saw the registration-pending page while no account was created and no verification email sent. Serialization failures are now retried in a fresh transaction (up to 3 attempts) — a genuine same-email race still returns the 409/anti-enumeration response, and exhausted retries surface as an error instead of a false success. Affects all prior versions; found via the demo app's concurrent Playwright suite. - `user.security.rememberMe.usePersistentTokens` was only honored in its exact camelCase spelling; the kebab-case spelling advertised by the generated configuration metadata (`user.security.remember-me.use-persistent-tokens`) bound the properties bean but never created the persistent-token repository, silently downgrading remember-me to hash-based tokens (which cannot be revoked server-side). The condition now accepts every relaxed spelling. ## [5.2.0] - 2026-08-12 diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java b/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java index 4f662c6a..e7d14919 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java @@ -208,6 +208,12 @@ public String getValue() { /** The user role name. */ private static final String USER_ROLE_NAME = "ROLE_USER"; + /** Attempts for the SERIALIZABLE registration write before a serialization failure is surfaced. */ + private static final int REGISTRATION_SERIALIZATION_ATTEMPTS = 5; + + /** Base delay between registration serialization retries; the actual delay grows per attempt and is jittered. */ + private static final long REGISTRATION_RETRY_BASE_DELAY_MS = 25; + /** The user repository. */ private final UserRepository userRepository; @@ -288,13 +294,15 @@ public String getValue() { * @param newUserDto the data transfer object containing the user registration * information *

- * Runs with {@link Isolation#SERIALIZABLE} isolation to close the duplicate-registration - * race when two requests register the same email concurrently. The {@link #emailExists} - * pre-check handles the common case, but a concurrent insert can still fail at commit; in - * that case the resulting {@link DataIntegrityViolationException} (unique-constraint - * violation) or serialization failure ({@link CannotAcquireLockException} / - * {@link ConcurrencyFailureException}) is translated into a {@link UserAlreadyExistException} - * (HTTP 409) rather than surfacing as a 500. Unrelated failures are never swallowed. + * The DB write runs with {@link Isolation#SERIALIZABLE} isolation to close the + * duplicate-registration race when two requests register the same email concurrently: a losing + * duplicate insert ({@link DataIntegrityViolationException}) is translated into a + * {@link UserAlreadyExistException} (HTTP 409). A serialization failure + * ({@link CannotAcquireLockException} / {@link ConcurrencyFailureException}) — which can also be + * caused by a concurrent registration of a different email deadlocking on index gap locks + * — is retried in a fresh transaction (see {@link #persistWithSerializationRetry}); exhausted + * retries propagate the failure rather than misreporting it as an existing account. Unrelated + * failures are never swallowed. *

* * @implNote This method is {@link Propagation#NOT_SUPPORTED}: the slow bcrypt hash runs with no @@ -346,12 +354,80 @@ public User registerNewUserAccount(final UserDto newUserDto) { // Persist through the proxy so the SERIALIZABLE transaction actually applies (a direct // this.persistNewUserAccount(...) self-invocation would bypass the proxy and run no transaction). - User saved = self.persistNewUserAccount(user); + User saved = persistWithSerializationRetry(user); // authWithoutPassword(saved); timeLogger.end(); return saved; } + /** + * Invokes {@link #persistNewUserAccount(User)} through the proxy, retrying when the SERIALIZABLE + * transaction fails to serialize (deadlock / lock-acquisition failure, + * {@link ConcurrencyFailureException}). + * + *

+ * A serialization failure does NOT imply a duplicate: two concurrent registrations of + * different emails can deadlock on index gap locks under SERIALIZABLE isolation. Each + * retry runs a fresh transaction whose {@code emailExists} pre-check distinguishes the two cases — + * a genuine same-email race throws {@link UserAlreadyExistException} (HTTP 409), while a + * different-email deadlock simply succeeds on retry. When every attempt fails to serialize, the + * last {@link ConcurrencyFailureException} propagates so the failure is visible to the caller + * (HTTP 500) instead of being misreported as an existing account while no account was created. + *

+ * + * @param user the fully built user entity (password already encoded) + * @return the saved user entity + * @throws UserAlreadyExistException if an account with the same email already exists + * @throws ConcurrencyFailureException if every attempt fails to serialize + */ + private User persistWithSerializationRetry(final User prototype) { + ConcurrencyFailureException lastFailure = null; + for (int attempt = 1; attempt <= REGISTRATION_SERIALIZATION_ATTEMPTS; attempt++) { + try { + // Each attempt persists a FRESH entity: a rolled-back attempt can leave the passed instance + // carrying persistence state (a generated id, Hibernate-managed collections such as + // passwordHistoryEntries), and re-saving that instance fails with optimistic-locking or + // orphan-delete errors instead of performing a clean INSERT. + return self.persistNewUserAccount(copyForInsert(prototype)); + } catch (ConcurrencyFailureException e) { + lastFailure = e; + log.warn("UserService.persistWithSerializationRetry: serialization failure on attempt {}/{} for email {}: {}", + attempt, REGISTRATION_SERIALIZATION_ATTEMPTS, prototype.getEmail(), e.getClass().getSimpleName()); + if (attempt < REGISTRATION_SERIALIZATION_ATTEMPTS) { + try { + // Growing, jittered delay: concurrent losers retrying in lockstep would keep + // deadlocking against each other; jitter desynchronizes them. + long delay = REGISTRATION_RETRY_BASE_DELAY_MS * attempt + + java.util.concurrent.ThreadLocalRandom.current().nextLong(REGISTRATION_RETRY_BASE_DELAY_MS * attempt); + Thread.sleep(delay); + } catch (InterruptedException interrupted) { + Thread.currentThread().interrupt(); + throw lastFailure; + } + } + } + } + throw lastFailure; + } + + /** + * Copies the registration-relevant fields onto a new transient {@link User} for a persist attempt. + * Only the fields set by {@link #registerNewUserAccount(UserDto)} are copied; everything else keeps + * its entity default, exactly as on a first attempt. + * + * @param prototype the user carrying the registration data + * @return a fresh transient copy safe to persist + */ + private static User copyForInsert(final User prototype) { + User user = new User(); + user.setFirstName(prototype.getFirstName()); + user.setLastName(prototype.getLastName()); + user.setPassword(prototype.getPassword()); + user.setEmail(prototype.getEmail()); + user.setEnabled(prototype.isEnabled()); + return user; + } + /** * Persists a new user account inside a short, serializable transaction. * @@ -361,10 +437,11 @@ public User registerNewUserAccount(final UserDto newUserDto) { * connection-holding transaction is open. It runs with {@link Isolation#SERIALIZABLE} to close the * duplicate-registration race when two requests register the same email concurrently. The * {@link #emailExists} pre-check handles the common case, but a concurrent insert can still fail at - * commit; in that case the resulting {@link DataIntegrityViolationException} (unique-constraint - * violation) or serialization failure ({@link CannotAcquireLockException} / - * {@link ConcurrencyFailureException}) is translated into a {@link UserAlreadyExistException} - * (HTTP 409) rather than surfacing as a 500. Unrelated failures are never swallowed. + * commit: a unique-constraint violation ({@link DataIntegrityViolationException}) is translated + * into a {@link UserAlreadyExistException} (HTTP 409), while a serialization failure + * ({@link CannotAcquireLockException} / {@link ConcurrencyFailureException}) propagates unchanged + * so the caller's retry ({@link #persistWithSerializationRetry}) can distinguish a same-email race + * from a different-email deadlock. Unrelated failures are never swallowed. *

* *

@@ -397,13 +474,14 @@ protected User persistNewUserAccount(final User user) { User saved = userRepository.save(user); savePasswordHistory(saved, saved.getPassword()); return saved; - } catch (DataIntegrityViolationException | ConcurrencyFailureException e) { - // A concurrent registration won the race: the unique-email constraint was violated - // (DataIntegrityViolationException) or the SERIALIZABLE transaction could not be - // serialized (ConcurrencyFailureException, e.g. CannotAcquireLockException). Translate - // to a 409 instead of letting it surface as a 500. Only these duplicate/serialization - // cases are translated; unrelated exceptions propagate unchanged. - log.debug("UserService.persistNewUserAccount: concurrent registration detected for email {}: {}", + } catch (DataIntegrityViolationException e) { + // A concurrent registration of the SAME email won the race: the unique-email constraint + // was violated. Translate to a 409 instead of letting it surface as a 500. A + // ConcurrencyFailureException (deadlock / serialization failure) is deliberately NOT + // translated here: it can be caused by a concurrent registration of a DIFFERENT email, + // so it propagates to the retry in persistWithSerializationRetry, whose fresh-transaction + // pre-check distinguishes the two cases. Unrelated exceptions propagate unchanged. + log.debug("UserService.persistNewUserAccount: concurrent duplicate registration detected for email {}: {}", user.getEmail(), e.getClass().getSimpleName()); throw new UserAlreadyExistException( "There is an account with that email address: " + user.getEmail()); diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/AbstractConcurrentRegistrationTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/AbstractConcurrentRegistrationTest.java index 4d65f03f..57039716 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/AbstractConcurrentRegistrationTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/AbstractConcurrentRegistrationTest.java @@ -25,8 +25,11 @@ /** * Validates that the SERIALIZABLE duplicate-registration race protection (UserService.registerNewUserAccount -> - * persistNewUserAccount, isolation = SERIALIZABLE, with DataIntegrityViolationException / ConcurrencyFailureException - * translated to UserAlreadyExistException) actually holds on a real, production-grade database — not just on H2. + * persistNewUserAccount, isolation = SERIALIZABLE, with DataIntegrityViolationException translated to + * UserAlreadyExistException and serialization failures retried in a fresh transaction) actually holds on a real, + * production-grade database — not just on H2. Also validates the inverse: concurrent registrations of DIFFERENT + * emails, which can deadlock on index gap locks under SERIALIZABLE, must all succeed via the retry rather than + * being misreported as duplicates. * *

* Two threads race to register the SAME email at the same instant (released together via a CountDownLatch). On a real @@ -131,6 +134,53 @@ void shouldSerializeConcurrentDuplicateRegistrationWhenTwoThreadsRaceSameEmail() } } + @RepeatedTest(value = 3, name = "{displayName} [run {currentRepetition}/{totalRepetitions}]") + @DisplayName("should register every user when threads race with different emails") + void shouldRegisterEveryUserWhenThreadsRaceDifferentEmails() throws InterruptedException { + // Distinct emails cannot conflict logically, but their SERIALIZABLE transactions can still deadlock on + // index gap locks. Before the serialization retry existed, the deadlock victim was misreported as + // UserAlreadyExistException — a silently lost registration behind the anti-enumeration success page. + final int threadCount = 6; + final CountDownLatch readyLatch = new CountDownLatch(threadCount); + final CountDownLatch startLatch = new CountDownLatch(1); + final ExecutorService executor = Executors.newFixedThreadPool(threadCount); + + try { + final List emails = new ArrayList<>(); + final List> futures = new ArrayList<>(); + for (int i = 0; i < threadCount; i++) { + final String email = "distinct-" + i + "-" + System.nanoTime() + "@test.com"; + emails.add(email); + futures.add(executor.submit(registrationTask(email, readyLatch, startLatch))); + } + + assertThat(readyLatch.await(30, TimeUnit.SECONDS)) + .as("all registration threads should reach the start gate") + .isTrue(); + startLatch.countDown(); + + final List failures = new ArrayList<>(); + for (Future future : futures) { + final RegistrationOutcome outcome = collect(future); + if (outcome.user == null) { + failures.add(outcome.error); + } + } + + assertThat(failures) + .as("every distinct-email registration must succeed — a deadlock between them must be retried, " + + "never surfaced (and never misreported as UserAlreadyExistException)") + .isEmpty(); + for (String email : emails) { + assertThat(userRepository.findByEmail(email.toLowerCase())) + .as("user row should exist for %s", email) + .isNotNull(); + } + } finally { + executor.shutdownNow(); + } + } + private Callable registrationTask(final String email, final CountDownLatch readyLatch, final CountDownLatch startLatch) { return () -> { diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java index 05ca275b..89bb62c5 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java @@ -183,13 +183,37 @@ void registerNewUserAccount_translatesDataIntegrityViolationToUserAlreadyExist() } @Test - @DisplayName("registerNewUserAccount - translates serialization failure (ConcurrencyFailureException) into UserAlreadyExistException") - void registerNewUserAccount_translatesConcurrencyFailureToUserAlreadyExist() { - // Given: pre-check passes but the SERIALIZABLE transaction cannot acquire the lock at commit + @DisplayName("registerNewUserAccount - retries and succeeds when a serialization failure is transient (different-email deadlock)") + void shouldRetryAndSucceedWhenSerializationFailureIsTransient() { + // Given: pre-check passes; the first SERIALIZABLE attempt deadlocks (e.g. against a concurrent + // registration of a DIFFERENT email), the retry succeeds. Before the retry existed this was + // misreported as UserAlreadyExistException and the registration silently lost. Role userRole = RoleTestDataBuilder.aUserRole().build(); when(passwordEncoder.encode(anyString())).thenReturn("encodedPassword"); when(roleRepository.findByName(USER_ROLE_NAME)).thenReturn(userRole); when(userRepository.findByEmail(anyString())).thenReturn(null); + when(userRepository.save(any(User.class))) + .thenThrow(new CannotAcquireLockException("deadlock")) + .thenAnswer(invocation -> invocation.getArgument(0)); + + // When + User saved = userService.registerNewUserAccount(testUserDto); + + // Then + assertThat(saved).isNotNull(); + assertThat(saved.getEmail()).isEqualTo(testUserDto.getEmail()); + verify(userRepository, org.mockito.Mockito.times(2)).save(any(User.class)); + } + + @Test + @DisplayName("registerNewUserAccount - throws UserAlreadyExistException when the retry finds a concurrent duplicate") + void shouldThrowUserAlreadyExistWhenRetryFindsConcurrentDuplicate() { + // Given: the first attempt fails to serialize because a concurrent registration of the SAME + // email won the race; on retry the pre-check sees the winner's committed row. + Role userRole = RoleTestDataBuilder.aUserRole().build(); + when(passwordEncoder.encode(anyString())).thenReturn("encodedPassword"); + when(roleRepository.findByName(USER_ROLE_NAME)).thenReturn(userRole); + when(userRepository.findByEmail(anyString())).thenReturn(null).thenReturn(testUser); when(userRepository.save(any(User.class))) .thenThrow(new CannotAcquireLockException("could not serialize access")); @@ -199,6 +223,24 @@ void registerNewUserAccount_translatesConcurrencyFailureToUserAlreadyExist() { .hasMessageContaining("There is an account with that email address"); } + @Test + @DisplayName("registerNewUserAccount - propagates ConcurrencyFailureException when retries are exhausted") + void shouldPropagateConcurrencyFailureWhenRetriesExhausted() { + // Given: every SERIALIZABLE attempt deadlocks. The failure must surface honestly (a 500 the + // caller can see and the user can retry), never as a fake "already exists" success/409. + Role userRole = RoleTestDataBuilder.aUserRole().build(); + when(passwordEncoder.encode(anyString())).thenReturn("encodedPassword"); + when(roleRepository.findByName(USER_ROLE_NAME)).thenReturn(userRole); + when(userRepository.findByEmail(anyString())).thenReturn(null); + when(userRepository.save(any(User.class))) + .thenThrow(new CannotAcquireLockException("persistent deadlock")); + + // When & Then + assertThatThrownBy(() -> userService.registerNewUserAccount(testUserDto)) + .isInstanceOf(org.springframework.dao.ConcurrencyFailureException.class); + verify(userRepository, org.mockito.Mockito.times(5)).save(any(User.class)); + } + @Test @DisplayName("registerNewUserAccount - does not swallow unrelated runtime exceptions from save") void registerNewUserAccount_doesNotSwallowUnrelatedExceptions() { From 00f99bb9a5b8356785cd79b12ae07c23f28cb120 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Fri, 14 Aug 2026 22:40:57 -0600 Subject: [PATCH 2/2] test: retry FK-racing test-user cleanup in CAPTCHA/UserApi integration tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The @Async RegistrationListener (default application executor, not the dsMailExecutor the CAPTCHA test drains) can commit a verification token between the cleanup transaction's deleteByUser and the user delete, failing tearDown on the FK constraint — seen on PR #357 CI. Retry the cleanup transaction so a fresh attempt sees and deletes the late token. --- .../spring/user/api/UserApiTest.java | 32 +++++++++++++---- .../CaptchaProtectionIntegrationTest.java | 34 +++++++++++++++---- 2 files changed, 52 insertions(+), 14 deletions(-) diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java index 8128fc99..874d20ca 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java @@ -190,14 +190,32 @@ void tearDown() { */ private void deleteTestUser(String email) { // This test is not @Transactional, so cleanup must run in its own committed transaction. - txTemplate.executeWithoutResult(status -> { - User user = userRepository.findByEmail(email); - if (user != null) { - passwordResetTokenRepository.deleteByUser(user); - verificationTokenRepository.deleteByUser(user); - userRepository.delete(user); + // Retried because the @Async RegistrationListener can commit a verification token between + // deleteByUser and the user delete, failing the FK constraint; the retry's fresh + // transaction sees and deletes the late token. + for (int attempt = 1;; attempt++) { + try { + txTemplate.executeWithoutResult(status -> { + User user = userRepository.findByEmail(email); + if (user != null) { + passwordResetTokenRepository.deleteByUser(user); + verificationTokenRepository.deleteByUser(user); + userRepository.delete(user); + } + }); + return; + } catch (org.springframework.dao.DataIntegrityViolationException e) { + if (attempt >= 3) { + throw e; + } + try { + Thread.sleep(100); + } catch (InterruptedException interrupted) { + Thread.currentThread().interrupt(); + throw e; + } } - }); + } } private String json(Object value) { diff --git a/src/test/java/com/digitalsanctuary/spring/user/captcha/CaptchaProtectionIntegrationTest.java b/src/test/java/com/digitalsanctuary/spring/user/captcha/CaptchaProtectionIntegrationTest.java index bf52b7c8..34855a0b 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/captcha/CaptchaProtectionIntegrationTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/captcha/CaptchaProtectionIntegrationTest.java @@ -220,16 +220,36 @@ private void drainMailExecutor() { * Hard-deletes the test user and any associated tokens (tokens first, FK order). This test is * not @Transactional, so cleanup runs in its own committed transaction — same pattern as * UserApiTest. + * + *

Retried because the token insert happens on the {@code @Async} RegistrationListener's + * executor (not {@code dsMailExecutor}, which {@code drainMailExecutor()} waits on), so a + * verification token can commit between {@code deleteByUser} and the user delete and fail the + * FK constraint; the retry's fresh transaction sees and deletes it.

*/ private void deleteTestUser(String email) { - txTemplate.executeWithoutResult(status -> { - User user = userRepository.findByEmail(email); - if (user != null) { - passwordResetTokenRepository.deleteByUser(user); - verificationTokenRepository.deleteByUser(user); - userRepository.delete(user); + for (int attempt = 1;; attempt++) { + try { + txTemplate.executeWithoutResult(status -> { + User user = userRepository.findByEmail(email); + if (user != null) { + passwordResetTokenRepository.deleteByUser(user); + verificationTokenRepository.deleteByUser(user); + userRepository.delete(user); + } + }); + return; + } catch (org.springframework.dao.DataIntegrityViolationException e) { + if (attempt >= 3) { + throw e; + } + try { + Thread.sleep(100); + } catch (InterruptedException interrupted) { + Thread.currentThread().interrupt(); + throw e; + } } - }); + } } /**