- * 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/api/UserApiTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java index 8128fc9..874d20c 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 bf52b7c..34855a0 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; + } } - }); + } } /** 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 4d65f03..5703971 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