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.", + ) } 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");