fix(store): return 409 for idempotency key reuse, not 422

Both idempotency paths returned a bare fmt.Errorf, and writeDomainError
maps anything it does not recognise to its 422 "invalid_request"
default. So reusing a key with a different payload answered 422 where
openapi.json declares 409 and state-transitions.json requires
"reject_conflict_without_state_change".

That is the difference between "your request was malformed" and "that
key is taken". A client acting on 422 would rewrite a request that was
never wrong, and the 409 branch of every generated client was
unreachable.

Wrap domain.ErrConflict on both the create and mutate paths, and add an
integration test covering identical replay and conflicting reuse.

Pre-existing: both bare errors are unchanged from 089c127c, which is why
verify-allocated-compose failed in CI before this branch's work as well.
Found only after adding the diagnostics in 432e5a11 and fc2f5c86 --
until then the assertion aborted silently and three CI runs reported
nothing but "make: *** Error 1".
This commit is contained in:
Josh Creek
2026-09-05 19:30:41 +01:00
parent fc2f5c8669
commit 14da286e11
2 changed files with 47 additions and 2 deletions
+40
View File
@@ -2562,3 +2562,43 @@ func TestPostgreSQLSessionRevocationIsImmediateOnAnotherReplica(t *testing.T) {
t.Fatalf("revoking one session invalidated another: %v", err) t.Fatalf("revoking one session invalidated another: %v", err)
} }
} }
// Reusing an idempotency key with a different payload must be a conflict.
// Both idempotency paths returned a bare error, which writeDomainError maps
// to its 422 default, so the API answered 422 where openapi.json and
// state-transitions.json ("same_key_different_payload":
// "reject_conflict_without_state_change") both require 409. It is the
// difference between "your request was malformed" and "that key is taken",
// and a client acting on the former would rewrite a correct request.
func TestPostgreSQLIdempotencyKeyReuseIsAConflict(t *testing.T) {
db := openIntegrationPostgres(t)
applyIntegrationMigrations(t, db)
ctx := context.Background()
now := time.Now().UTC().Truncate(time.Microsecond)
if _, err := db.ExecContext(ctx, `INSERT INTO identities (player_id, steam_id) VALUES ('idem-player', 'idem-steam')`); err != nil {
t.Fatal(err)
}
spec := domain.QueueSpec{Playlist: domain.Casual, ClientBuild: "build-1", ProtocolVersion: 1}
if _, err := CreateQueueTicket(ctx, db, "idem-ticket-one", "idem-player", "idem-key-00000001", spec, now); err != nil {
t.Fatalf("first create: %v", err)
}
// Same key, same payload: replays the original result.
replay, err := CreateQueueTicket(ctx, db, "idem-ticket-one", "idem-player", "idem-key-00000001", spec, now.Add(time.Second))
if err != nil {
t.Fatalf("identical replay must succeed: %v", err)
}
if replay.TicketID != "idem-ticket-one" {
t.Fatalf("replay returned %q", replay.TicketID)
}
// Same key, different payload: conflict, not a validation error.
_, err = CreateQueueTicket(ctx, db, "idem-ticket-two", "idem-player", "idem-key-00000001", spec, now.Add(time.Second))
if err == nil {
t.Fatal("reusing a key with a different ticket was accepted")
}
if !errors.Is(err, domain.ErrConflict) {
t.Fatalf("err = %v; must wrap domain.ErrConflict so the API answers 409, not its 422 default", err)
}
}
+7 -2
View File
@@ -178,7 +178,12 @@ func CreateQueueTicket(ctx context.Context, db *sql.DB, ticketID, playerID, idem
return err return err
} }
if !bytes.Equal(priorDigest, digest[:]) { if !bytes.Equal(priorDigest, digest[:]) {
return fmt.Errorf("queue create idempotency conflict") // Must wrap ErrConflict: writeDomainError maps unrecognised
// errors to 422, but the contract
// (state-transitions.json "same_key_different_payload") and
// openapi.json both require 409 for reusing a key with a
// different payload.
return fmt.Errorf("%w: queue create idempotency conflict", domain.ErrConflict)
} }
var prior queueTicketRecord var prior queueTicketRecord
if err := json.Unmarshal(priorResult, &prior); err != nil { if err := json.Unmarshal(priorResult, &prior); err != nil {
@@ -321,7 +326,7 @@ func mutateQueueTicket(ctx context.Context, db *sql.DB, playerID, ticketID, idem
return err return err
} }
if !bytes.Equal(priorDigest, digest[:]) { if !bytes.Equal(priorDigest, digest[:]) {
return fmt.Errorf("queue mutation idempotency conflict") return fmt.Errorf("%w: queue mutation idempotency conflict", domain.ErrConflict)
} }
var prior queueTicketRecord var prior queueTicketRecord
if err := json.Unmarshal(priorResult, &prior); err != nil { if err := json.Unmarshal(priorResult, &prior); err != nil {