Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions build-logic/src/main/kotlin/pkauth.java-conventions.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<UserItem> table;
private final DynamoDbIndex<UserItem> 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);
}
Expand Down Expand Up @@ -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#<lowercased>) is the only guard that actually enforces one handle per
// username: the user row's own pk is USER#<random handle>, 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;
}
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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#<lowercased>}, {@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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Future<UserHandle>> 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<UserHandle> distinct = new HashSet<>();
for (Future<UserHandle> 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");
Expand Down
Loading