From f7a684d9efaea85e2603332410f34026f1b4013f Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Mon, 20 Jul 2026 07:32:17 +0000 Subject: [PATCH 1/2] Fix DynamoDB KV CAS not retrying on transactional conditional check failures The Batch() error detection in the DynamoDB KV store only matched ConditionalCheckFailedException, which DynamoDB returns for single-item operations. TransactWriteItems reports condition failures as TransactionCanceledException with a ConditionalCheckFailed cancellation reason, so errors.As never matched, retry=false was returned, and the CAS loop gave up on the first attempt instead of re-reading the ring and retrying. In practice this means every ring write conflict on the DynamoDB KV store is treated as fatal. During rolling updates where many ingesters join/leave concurrently, a joining ingester that loses the optimistic concurrency race fails startup with 'failed to pick tokens in the KV store' and crashes, instead of retrying with a fresh read. Detect TransactionCanceledException and inspect CancellationReasons for ConditionalCheckFailed. TransactionConflict reasons (concurrent transaction on the same item) are equally transient and also treated as retryable. Signed-off-by: Ben Ye --- CHANGELOG.md | 1 + pkg/ring/kv/dynamodb/dynamodb.go | 37 +++++++++++++++++++++++++-- pkg/ring/kv/dynamodb/dynamodb_test.go | 32 +++++++++++++++++++++++ 3 files changed, 68 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 47dbba996eb..3287db55cad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -84,6 +84,7 @@ * [BUGFIX] Querier: Fix parquet queryable fallback returning a nil error instead of the actual query error in `LabelValues` and `LabelNames`. #7638 * [BUGFIX] Storage: Default the Azure `endpoint_suffix` to `blob.core.windows.net` instead of empty. Cortex builds the Thanos Azure config directly and bypasses Thanos' default, so an unset suffix produced an invalid FQDN (`.`) and components hung on startup with DNS errors. #5449 * [BUGFIX] Store Gateway: Fix misleading "no index cache backend addresses" validation error being reported for chunks-cache, metadata-cache, and parquet caches when their memcached or redis backend is configured without addresses. The message is now the cache-type-agnostic "no cache backend addresses". #7675 +* [BUGFIX] Ring: Fix DynamoDB KV CAS not retrying on transactional conditional check failures. `TransactWriteItems` reports condition failures as `TransactionCanceledException` with a `ConditionalCheckFailed` cancellation reason, which was not recognized as retryable, so any concurrent ring update conflict (e.g. many ingesters joining during a rolling update) failed immediately instead of re-reading and retrying. `TransactionConflict` cancellation reasons are also treated as retryable. #7706 ## 1.21.0 2026-04-24 diff --git a/pkg/ring/kv/dynamodb/dynamodb.go b/pkg/ring/kv/dynamodb/dynamodb.go index 9bc4e99e30d..d4604611640 100644 --- a/pkg/ring/kv/dynamodb/dynamodb.go +++ b/pkg/ring/kv/dynamodb/dynamodb.go @@ -265,8 +265,7 @@ func (kv dynamodbKV) Batch(ctx context.Context, put map[dynamodbKey]dynamodbItem TransactItems: slice, }) if err != nil { - var checkFailed *types.ConditionalCheckFailedException - isCheckFailed := errors.As(err, &checkFailed) + isCheckFailed := isConditionalCheckFailure(err) if isCheckFailed { kv.logger.Log("msg", "conditional check failed on DynamoDB Batch", "err", err) } @@ -280,6 +279,40 @@ func (kv dynamodbKV) Batch(ctx context.Context, put map[dynamodbKey]dynamodbItem return totalCapacity, false, nil } +// isConditionalCheckFailure returns true if the given error was caused by a +// conditional check failure, meaning another writer updated the item between +// our read and write. Such conflicts are retryable: the caller should re-read +// the latest state and retry the operation. +// +// DynamoDB reports conditional check failures differently depending on the API: +// - Single-item operations (PutItem, UpdateItem, DeleteItem) return +// ConditionalCheckFailedException. +// - TransactWriteItems returns TransactionCanceledException with the failure +// reported in CancellationReasons with code "ConditionalCheckFailed". +// +// TransactionConflict cancellation reasons (another transaction is in progress +// on the same item) are equally transient and also treated as retryable. +func isConditionalCheckFailure(err error) bool { + var checkFailed *types.ConditionalCheckFailedException + if errors.As(err, &checkFailed) { + return true + } + + var txnCanceled *types.TransactionCanceledException + if errors.As(err, &txnCanceled) { + for _, reason := range txnCanceled.CancellationReasons { + if reason.Code == nil { + continue + } + switch *reason.Code { + case "ConditionalCheckFailed", "TransactionConflict": + return true + } + } + } + return false +} + func (kv dynamodbKV) generatePutItemRequest(key dynamodbKey, ddbItem dynamodbItem) map[string]types.AttributeValue { item := generateItemKey(key) item[contentData] = &types.AttributeValueMemberB{Value: ddbItem.data} diff --git a/pkg/ring/kv/dynamodb/dynamodb_test.go b/pkg/ring/kv/dynamodb/dynamodb_test.go index 065e38c6442..54419c603ec 100644 --- a/pkg/ring/kv/dynamodb/dynamodb_test.go +++ b/pkg/ring/kv/dynamodb/dynamodb_test.go @@ -170,6 +170,38 @@ func Test_Batch_Error(t *testing.T) { mockError: &types.ConditionalCheckFailedException{}, expectedRetry: true, }, + { + // TransactWriteItems reports conditional check failures as + // TransactionCanceledException with a ConditionalCheckFailed + // cancellation reason, wrapped by the SDK operation error. + name: "transaction_canceled_conditional_check_failed_should_retry", + mockError: fmt.Errorf("operation error DynamoDB: TransactWriteItems: %w", &types.TransactionCanceledException{ + Message: aws.String("Transaction cancelled, please refer cancellation reasons for specific reasons"), + CancellationReasons: []types.CancellationReason{ + {Code: aws.String("None")}, + {Code: aws.String("ConditionalCheckFailed")}, + }, + }), + expectedRetry: true, + }, + { + name: "transaction_canceled_transaction_conflict_should_retry", + mockError: fmt.Errorf("operation error DynamoDB: TransactWriteItems: %w", &types.TransactionCanceledException{ + CancellationReasons: []types.CancellationReason{ + {Code: aws.String("TransactionConflict")}, + }, + }), + expectedRetry: true, + }, + { + name: "transaction_canceled_other_reason_no_retry", + mockError: fmt.Errorf("operation error DynamoDB: TransactWriteItems: %w", &types.TransactionCanceledException{ + CancellationReasons: []types.CancellationReason{ + {Code: aws.String("ValidationError")}, + }, + }), + expectedRetry: false, + }, } for _, tc := range testCases { From 1b4640ebbd07e3fa6bf053a14850df13dff903de Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Mon, 20 Jul 2026 07:41:47 +0000 Subject: [PATCH 2/2] Use errors.AsType to satisfy check-modernize Signed-off-by: Ben Ye --- pkg/ring/kv/dynamodb/dynamodb.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/ring/kv/dynamodb/dynamodb.go b/pkg/ring/kv/dynamodb/dynamodb.go index d4604611640..dc33301851c 100644 --- a/pkg/ring/kv/dynamodb/dynamodb.go +++ b/pkg/ring/kv/dynamodb/dynamodb.go @@ -293,13 +293,11 @@ func (kv dynamodbKV) Batch(ctx context.Context, put map[dynamodbKey]dynamodbItem // TransactionConflict cancellation reasons (another transaction is in progress // on the same item) are equally transient and also treated as retryable. func isConditionalCheckFailure(err error) bool { - var checkFailed *types.ConditionalCheckFailedException - if errors.As(err, &checkFailed) { + if _, ok := errors.AsType[*types.ConditionalCheckFailedException](err); ok { return true } - var txnCanceled *types.TransactionCanceledException - if errors.As(err, &txnCanceled) { + if txnCanceled, ok := errors.AsType[*types.TransactionCanceledException](err); ok { for _, reason := range txnCanceled.CancellationReasons { if reason.Code == nil { continue