From accf923e62aedd66fa2f96c488641e3a5399dd0c Mon Sep 17 00:00:00 2001 From: Ned Wolpert Date: Fri, 3 Jul 2026 07:40:50 -0700 Subject: [PATCH 1/2] fix(dynamodb): enforce username uniqueness in getOrCreateHandle DynamoDbUserLookup.getOrCreateHandle did a read-then-conditional-put whose `attribute_not_exists(pk)` condition guarded the random-handle key (USER#), not the username, and a GSI cannot enforce uniqueness. Two concurrent first-time getOrCreateHandle calls for the same username therefore both minted a handle and both wrote successfully, leaving two user rows sharing USERNAME#. Lookups (findFirst over the unenforced GSI) then resolved non-deterministically, splitting one identity across two handles and stranding a registered passkey at login. Write the user row and a dedicated username-uniqueness marker (pk = USERNAME#) atomically via TransactWriteItems, each with attribute_not_exists(pk). The marker is the real guard: only one racer can create USERNAME#, so the loser's transaction is cancelled. On TransactionCanceledException, recover the winner's handle from the marker with a strongly-consistent read (the username GSI is only eventually consistent and may lag); if no marker exists the cancellation was transient, so rethrow rather than return an unpersisted handle. The marker leaves gsi1pk unset so the sparse username GSI never indexes it. Mirrors the JDBI backend's ON CONFLICT (username) path. Adds an 8-thread concurrent-race test asserting all racers converge on one handle and the persisted lookup agrees. Co-Authored-By: Claude Opus 4.8 --- .../dynamodb/DynamoDbUserLookup.java | 64 +++++++++++++++---- .../pkauth/persistence/dynamodb/UserItem.java | 17 +++++ .../DynamoDbUserLookupIntegrationTest.java | 38 +++++++++++ 3 files changed, 108 insertions(+), 11 deletions(-) diff --git a/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbUserLookup.java b/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbUserLookup.java index 7cfc22b..b06a496 100644 --- a/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbUserLookup.java +++ b/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbUserLookup.java @@ -13,18 +13,23 @@ import software.amazon.awssdk.enhanced.dynamodb.Expression; import software.amazon.awssdk.enhanced.dynamodb.Key; import software.amazon.awssdk.enhanced.dynamodb.TableSchema; +import software.amazon.awssdk.enhanced.dynamodb.model.GetItemEnhancedRequest; import software.amazon.awssdk.enhanced.dynamodb.model.QueryConditional; -import software.amazon.awssdk.services.dynamodb.model.ConditionalCheckFailedException; +import software.amazon.awssdk.enhanced.dynamodb.model.TransactPutItemEnhancedRequest; +import software.amazon.awssdk.enhanced.dynamodb.model.TransactWriteItemsEnhancedRequest; +import software.amazon.awssdk.services.dynamodb.model.TransactionCanceledException; /** {@link UserLookup} backed by the separate {@code PkAuthUsers} table. */ public final class DynamoDbUserLookup implements UserLookup { + private final DynamoDbEnhancedClient enhanced; private final DynamoDbTable table; private final DynamoDbIndex byUsername; public DynamoDbUserLookup(DynamoDbEnhancedClient enhanced, PkAuthDynamoTables tables) { Objects.requireNonNull(enhanced, "enhanced"); Objects.requireNonNull(tables, "tables"); + this.enhanced = enhanced; this.table = enhanced.table(tables.users(), TableSchema.fromBean(UserItem.class)); this.byUsername = table.index(PkAuthDynamoTables.GSI1_USER_BY_USERNAME); } @@ -61,18 +66,55 @@ public UserHandle getOrCreateHandle(String username) { return UserHandle.of(Base64Url.decode(existing.get().getUserHandle())); } UserHandle handle = UserHandle.random(); - UserItem item = UserItem.build(handle, username, username); + // Write the user row AND a username-uniqueness marker atomically. The marker + // (pk = USERNAME#) is the only guard that actually enforces one handle per + // username: the user row's own pk is USER#, so a condition on it never + // collides on the username, and a GSI does not enforce uniqueness. Two concurrent + // getOrCreateHandle calls for one username both mint a handle, but only one + // TransactWriteItems can create the marker; the loser's transaction is cancelled. + UserItem userItem = UserItem.build(handle, username, username); + UserItem marker = UserItem.usernameMarker(handle, username); + Expression notExists = + Expression.builder().expression("attribute_not_exists(pk)").build(); try { - table.putItem( - r -> - r.item(item) - .conditionExpression( - Expression.builder().expression("attribute_not_exists(pk)").build())); + enhanced.transactWriteItems( + TransactWriteItemsEnhancedRequest.builder() + .addPutItem( + table, + TransactPutItemEnhancedRequest.builder(UserItem.class) + .item(userItem) + .conditionExpression(notExists) + .build()) + .addPutItem( + table, + TransactPutItemEnhancedRequest.builder(UserItem.class) + .item(marker) + .conditionExpression(notExists) + .build()) + .build()); return handle; - } catch (ConditionalCheckFailedException race) { - return lookupByUsername(username) - .map(u -> UserHandle.of(Base64Url.decode(u.getUserHandle()))) - .orElse(handle); + } catch (TransactionCanceledException race) { + // The username was claimed by a concurrent (or prior) creator. Recover the winner's + // handle from the marker with a STRONGLY-consistent read: the winning transaction wrote + // the marker atomically, so it is visible immediately, whereas the username GSI is only + // eventually consistent and may not reflect the winner's row yet. If no marker exists, + // the cancellation was transient (throughput / conflict), not a uniqueness clash, so + // rethrow — it surfaces as a 503 rather than returning an unpersisted handle. + UserItem winner = + table.getItem( + GetItemEnhancedRequest.builder() + .key( + Key.builder() + .partitionValue( + DynamoKeys.USERNAME + username.toLowerCase(Locale.ROOT)) + .sortValue("META") + .build()) + .consistentRead(true) + .build()); + if (winner != null) { + return UserHandle.of(Base64Url.decode(winner.getUserHandle())); + } + throw race; } }); } diff --git a/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/UserItem.java b/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/UserItem.java index 9f48841..946d849 100644 --- a/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/UserItem.java +++ b/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/UserItem.java @@ -116,6 +116,23 @@ public static UserItem build(UserHandle handle, String username, String displayN return item; } + /** + * Builds the username-uniqueness marker row: {@code pk = USERNAME#}, {@code sk = + * META}. A conditional put of this item ({@code attribute_not_exists(pk)}) is what actually + * enforces one handle per username — a GSI cannot, because GSIs do not enforce uniqueness. + * Deliberately leaves {@code gsi1pk} unset so the sparse username GSI does not index it (only the + * real {@link DynamoKeys#USER} row is indexed and returned by lookups); it carries {@code + * userHandle} so a racing loser can recover the winner's handle with a strongly-consistent read. + */ + static UserItem usernameMarker(UserHandle handle, String username) { + UserItem item = new UserItem(); + item.setPk(DynamoKeys.USERNAME + username.toLowerCase(java.util.Locale.ROOT)); + item.setSk("META"); + item.setUserHandle(Base64Url.encode(handle.value())); + item.setUsername(username); + return item; + } + /** Renders the row as a public {@link UserView}. */ public UserView toView() { return new UserView( diff --git a/pk-auth-persistence-dynamodb/src/test/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbUserLookupIntegrationTest.java b/pk-auth-persistence-dynamodb/src/test/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbUserLookupIntegrationTest.java index 2b420b5..71fde25 100644 --- a/pk-auth-persistence-dynamodb/src/test/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbUserLookupIntegrationTest.java +++ b/pk-auth-persistence-dynamodb/src/test/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbUserLookupIntegrationTest.java @@ -5,8 +5,16 @@ import com.codeheadsystems.pkauth.api.UserHandle; import com.codeheadsystems.pkauth.spi.UserLookup.UserView; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.UUID; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.condition.DisabledIfEnvironmentVariable; @@ -61,6 +69,36 @@ void findViewByHandleReturnsUsernameForKnownAndEmptyForUnknown() { assertThat(users.findViewByHandle(UserHandle.random())).isEmpty(); } + @Test + void concurrentGetOrCreateForSameUsernameConvergesOnOneHandle() throws Exception { + int threads = 8; + ExecutorService pool = Executors.newFixedThreadPool(threads); + CountDownLatch start = new CountDownLatch(1); + List> futures = new ArrayList<>(); + try { + for (int i = 0; i < threads; i++) { + futures.add( + pool.submit( + () -> { + start.await(); + return users.getOrCreateHandle("erin"); + })); + } + start.countDown(); // release all threads at once to maximise the race window + + Set distinct = new HashSet<>(); + for (Future f : futures) { + distinct.add(f.get()); + } + // The username-uniqueness marker forces every racer to converge on exactly one handle... + assertThat(distinct).hasSize(1); + // ...and the persisted lookup resolves to that same single handle (no split identity). + assertThat(users.findHandleByUsername("erin")).hasValue(distinct.iterator().next()); + } finally { + pool.shutdownNow(); + } + } + @Test void registerPersistsUsernameAndDisplayName() { UserHandle handle = users.register("dave", "Dave Display"); From a8b330f0b1b4bcbf7d8ed450e0b1a14bd46f7a35 Mon Sep 17 00:00:00 2001 From: Ned Wolpert Date: Fri, 3 Jul 2026 07:40:58 -0700 Subject: [PATCH 2/2] build: make spotless tasks incompatible with the configuration cache Spotless 8.x's google-java-format lazily loads Guava classes that fail to initialize (NoClassDefFoundError com/google/common/collect/ImmutableCollection) when the task work is served from a Gradle cache instead of running fresh. The build cache was already disabled repo-wide for this, but the configuration cache triggers the same crash on a cache-hit run (e.g. a second `check` or a clean `check`), leaving the gate red through no fault of the code. Extend the existing per-task cacheIf { false } workaround with notCompatibleWithConfigurationCache so the spotless tasks always run fresh. The configuration cache is retained for invocations that don't run spotless. Co-Authored-By: Claude Opus 4.8 --- .../kotlin/pkauth.java-conventions.gradle.kts | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/build-logic/src/main/kotlin/pkauth.java-conventions.gradle.kts b/build-logic/src/main/kotlin/pkauth.java-conventions.gradle.kts index 7f87a1b..9de2e5b 100644 --- a/build-logic/src/main/kotlin/pkauth.java-conventions.gradle.kts +++ b/build-logic/src/main/kotlin/pkauth.java-conventions.gradle.kts @@ -68,9 +68,19 @@ tasks.named("check") { } // Spotless + google-java-format intermittently fails class loading -// (`NoClassDefFoundError com/google/common/collect/ImmutableList$ReverseImmutableList`) when its -// outputs are restored from the Gradle build cache. Disabling cache hits for the spotless tasks -// avoids the bad-cache-state pathway; the formatter itself works correctly on a fresh run. +// (`NoClassDefFoundError com/google/common/collect/ImmutableCollection`) when its work is served +// from a Gradle cache instead of running fresh. This happens on BOTH cache pathways: +// - the build cache, on a cached-output hit (already mitigated repo-wide via +// `org.gradle.caching=false` in gradle.properties), and +// - the configuration cache, on a cache-hit run that restores the task without re-initializing +// the formatter's lazily-loaded Guava classes (e.g. a second `check` or a `clean check`). +// `outputs.cacheIf { false }` closes the first pathway per-task; marking the tasks not compatible +// with the configuration cache forces them to always run fresh, closing the second. The formatter +// itself is correct on a fresh run — this only removes the bad-cache-state pathways. tasks.matching { it.name.startsWith("spotless") }.configureEach { outputs.cacheIf { false } + notCompatibleWithConfigurationCache( + "Spotless google-java-format lazily loads Guava classes that fail to initialize when the" + + " task is restored from the configuration cache; must run fresh.", + ) }