fix: retry concurrent passwordless registration deadlocks (#368); document step-up session id behavior (#369)

[devondragon]

Summary

Closes #368 and #369.

#368 — Concurrent passwordless registrations no longer 500 on deadlock

registerPasswordlessAccount had the same SERIALIZABLE-isolation deadlock exposure as password registration but none of the hardening that path already got: it ran a single @Transactional(SERIALIZABLE) save with no retry and no DataIntegrityViolationException translation. A concurrent-insert deadlock (including between different emails contending on InnoDB index gap locks) surfaced as a generic 500 "System Error" on POST /user/registration/passwordless.

The fix mirrors the password path exactly:

• persistWithSerializationRetry is generalized to take the proxied persist call (Function<User, User>), so both registration paths share the retry-with-jittered-backoff loop.
• New persistNewPasswordlessAccount (@Transactional(SERIALIZABLE)) translates DataIntegrityViolationException to UserAlreadyExistException (409) and records no password history (the account has no password). ConcurrencyFailureException deliberately propagates to the retry, whose fresh-transaction pre-check distinguishes a same-email race (409) from a different-email deadlock (succeeds on retry).
• registerPasswordlessAccount is now Propagation.NOT_SUPPORTED so each retry runs in its own fresh short transaction. Exhausted retries propagate honestly (500 the caller can retry) rather than being misreported as an existing account.

#369 — Step-up session id behavior documented

Added a note to the CONFIG.md step-up section: re-assertion preserves the servlet session id (JSESSIONID) while rotating the CSRF token, because factor merging merges the new factor onto the already-authenticated principal instead of running a fresh authentication. This is intended (principal unchanged, no pre-auth session id to upgrade, so no fixation vector). Documents the client consequence: a client retrying a gated operation after step-up must refresh its CSRF token first.

Testing

• 4 new mock-based UserServiceTest cases for the passwordless path: retry-then-succeed, DataIntegrityViolationException→409, exhausted-retries→propagate ConcurrencyFailureException, and unrelated-exception-not-swallowed.
• Passwordless same-email and different-email cases added to AbstractConcurrentRegistrationTest, which runs against real MariaDB and PostgreSQL via Testcontainers. The MariaDB run reproduces the issue-368 deadlock (1213-40001) and confirms it is now caught and retried rather than surfaced.
• Full suite: 1280 tests, 0 failures.

https://claude.ai/code/session_01KL5qvKVGQLLQDjHToy34vj

[claude]

Review

This is a clean, well-executed mirror of the existing password-registration hardening onto the passwordless path. A few notes, none blocking.

Correctness — looks solid

• Moving the emailExists pre-check inside persistNewPasswordlessAccount's @Transactional(SERIALIZABLE) boundary (rather than checking outside the transaction as before) is the actual bug fix here — it closes a genuine TOCTOU window that existed in the old code, not just a cosmetic refactor.
• persistWithSerializationRetry generalized to Function<User, User> is a minimal, sensible way to share the retry loop between password and passwordless paths without duplicating the backoff/jitter logic.
• Correctly distinguishing DataIntegrityViolationException (same-email race → 409) from ConcurrencyFailureException (possibly different-email deadlock → retry) is preserved faithfully from the password path.
• copyForInsert re-copying a fresh transient User on each attempt (rather than re-saving the same instance) avoids the optimistic-locking/orphan-delete failure mode called out in the comment — good attention to detail.

One thing worth double-checking

• registerPasswordlessAccount moving to Propagation.NOT_SUPPORTED is documented clearly in the Javadoc (@implNote), including the consequence that an outer caller's transaction is suspended and won't roll back the persisted user. Given passwordless registration has no slow bcrypt work to keep off the connection (unlike the password path, where NOT_SUPPORTED protects against holding a connection during the encode), the primary reason for NOT_SUPPORTED here is really "let each retry run its own short transaction" rather than "avoid holding a connection during slow work." Worth confirming no existing consumer calls registerPasswordlessAccount from inside its own @Transactional expecting atomicity with other work — if any demo/consuming app does, this is a silent behavior change (documented, but only in Javadoc, not in CONFIG.md/MIGRATION.md).

Minor style nit

• persistWithSerializationRetry's Javadoc @param prototype says "or null for passwordless" — this appears to be leftover/inaccurate wording; the passwordless prototype has a null password, not a null prototype itself. Minor doc clarity issue only.

Test coverage — thorough

• The 4 new UserServiceTest mock cases hit the meaningful branches (retry-success, DIV→409, exhausted retries→propagate, unrelated exception not swallowed).
• Reusing AbstractConcurrentRegistrationTest against real MariaDB/PostgreSQL via Testcontainers for both same-email and different-email races is the right way to actually prove the deadlock scenario from Concurrent passwordless registrations deadlock on user_account insert (500 System Error) #368 is fixed, not just mocked away. Nice that the PR description notes the MariaDB run reproduces the original 1213-40001 deadlock.

Docs

• The CONFIG.md addition for Step-up re-authentication does not rotate the session id (CSRF token does rotate) #369 is a good, precise explanation of the session-id/CSRF-rotation asymmetry on step-up re-assertion, including the actionable client-side consequence (refresh CSRF token before retrying a gated operation post-step-up).

Overall: solid, low-risk fix that follows the codebase's established pattern faithfully. No security or performance concerns beyond the note above about NOT_SUPPORTED semantics for existing consumers.

[Copilot]

🟢 Approval recommended

The concurrency/transaction changes are consistent with the existing password registration hardening and are backed by targeted unit tests plus real concurrent DB tests.

Pull request overview

This PR hardens passwordless registration to handle SERIALIZABLE deadlocks the same way as password-based registration, and documents intended step-up session/CSRF behavior for clients consuming the library.

Changes:

• Generalizes the registration serialization retry loop to accept a proxied persister function and applies it to passwordless registration.
• Adds persistNewPasswordlessAccount(@Transactional(SERIALIZABLE)) to translate unique-constraint races into UserAlreadyExistException while allowing serialization failures to trigger retries.
• Adds unit + concurrent Testcontainers coverage for passwordless registration deadlock/duplicate scenarios and documents step-up session id vs CSRF rotation behavior.

File summaries

File	Description
src/main/java/com/digitalsanctuary/spring/user/service/UserService.java	Refactors retry helper to take a proxied persister and applies it to passwordless registration with a new SERIALIZABLE persist method and exception translation.
src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java	Adds mock-based tests validating passwordless retry behavior, exception translation, and propagation on exhausted retries.
src/test/java/com/digitalsanctuary/spring/user/service/AbstractConcurrentRegistrationTest.java	Adds real DB concurrent tests for passwordless same-email and different-email contention scenarios.
CONFIG.md	Documents intended step-up re-assertion behavior: session id unchanged while CSRF rotates, and client implications.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 0
• Review effort level: Lite

---

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