From ccbacfbdf9c051945fba507164879073fdec57e7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 17:09:24 +0200 Subject: [PATCH] refactor(aws): use the shared purchase guard in the MemoryDB purchase path Replace the MemoryDB-local copy of the issue #641 idempotency guard and AlreadyExists recovery with purchaseguard.Guard and Recover, as ElastiCache already does (#507). The lookup and fault type stay service-specific. Add purchase_guard_test.go covering each guard rule at this path: no lookup without a token, lookup before purchase, fail-loud lookup error, and AlreadyExists recovery limits (re-lookup error or miss, no token, other purchase errors). Mocks are bounded with Test(t). Part of #10 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + providers/aws/services/memorydb/client.go | 43 ++---- .../services/memorydb/purchase_guard_test.go | 137 ++++++++++++++++++ 3 files changed, 148 insertions(+), 33 deletions(-) create mode 100644 providers/aws/services/memorydb/purchase_guard_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e8785719..b6234c17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -133,6 +133,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Changed +- **`aws/memorydb`:** the MemoryDB purchase path uses the shared `purchaseguard` idempotency guard and AlreadyExists recovery, like ElastiCache. Behavior for valid purchases is unchanged; only the lookup-failure error wording changes to "before MemoryDB reservation purchase" (#10). - **`common.HoursPerMonth`:** exported 730-hour average month (`HoursPerYear / 12`) next to `HoursPerYear`; the remaining bare 730 and 8760 literals in the aws and gcp providers use the shared constants (#109). - **`gcp`:** region-lookup permission errors are now classified by type only (googleapi 403 or gRPC `PermissionDenied`) through the shared `IsPermissionError`. gRPC `PermissionDenied` is now skipped with a warning like a 403, and an untyped error whose text merely contains "403" and "permission" now propagates instead of being skipped (#6). - **BREAKING `gcp`: stored contexts removed (#6).** `computeengine.NewClient` diff --git a/providers/aws/services/memorydb/client.go b/providers/aws/services/memorydb/client.go index 7ddb80ff..fffdd703 100644 --- a/providers/aws/services/memorydb/client.go +++ b/providers/aws/services/memorydb/client.go @@ -16,6 +16,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/offeringprice" "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/purchasecfg" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/purchaseguard" "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/reservationexpiry" "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/reservationstate" "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/tagging" @@ -235,43 +236,19 @@ func (c *Client) findReservationByID(ctx context.Context, reservationID string) return "", false, nil } -// idempotencyGuard short-circuits a re-drive (issue #641): when token is set, it -// reports (existingID, true, nil) if a reserved node already exists under -// reservationID, ("", false, nil) for a first-time purchase, or a fail-loud -// error on lookup failure. With an empty token it is a no-op. +// idempotencyGuard short-circuits a re-drive (issue #641); see purchaseguard.Guard. func (c *Client) idempotencyGuard(ctx context.Context, token, reservationID string) (existingID string, found bool, err error) { - if token == "" { - return "", false, nil - } - existingID, found, lookupErr := c.findReservationByID(ctx, reservationID) - if lookupErr != nil { - return "", false, fmt.Errorf("idempotency lookup failed before MemoryDB purchase (refusing to purchase to avoid a possible double-buy): %w", lookupErr) - } - if found { - log.Printf("MemoryDB reservation for idempotency token %s already exists (%s); skipping purchase (issue #641 re-drive)", common.MaskToken(token), existingID) - return existingID, true, nil - } - return "", false, nil + return purchaseguard.Guard(ctx, token, "MemoryDB reservation", func(ctx context.Context) (string, bool, error) { + return c.findReservationByID(ctx, reservationID) + }) } -// recoverAlreadyExists handles the native server-side dedupe backstop (issue -// #641): if the by-ID guard missed the existing reservation but AWS rejected the -// duplicate ID with ReservedNodeAlreadyExistsFault, it re-Describes by ID and -// returns (existingID, true) so the re-drive recovers it instead of erroring. +// recoverAlreadyExists adopts the existing reservation when AWS rejects a +// duplicate ID (issue #641); see purchaseguard.Recover. func (c *Client) recoverAlreadyExists(ctx context.Context, token, reservationID string, purchaseErr error) (string, bool) { - if token == "" { - return "", false - } - var already *types.ReservedNodeAlreadyExistsFault - if !errors.As(purchaseErr, &already) { - return "", false - } - existingID, found, lookupErr := c.findReservationByID(ctx, reservationID) - if lookupErr == nil && found { - log.Printf("MemoryDB reservation %s already existed at purchase time; treating as idempotent re-drive (issue #641)", existingID) - return existingID, true - } - return "", false + return purchaseguard.Recover[*types.ReservedNodeAlreadyExistsFault](ctx, token, "MemoryDB reservation", purchaseErr, func(ctx context.Context) (string, bool, error) { + return c.findReservationByID(ctx, reservationID) + }) } // ReservedNodeEngine is the engine recorded on MemoryDB reserved nodes and diff --git a/providers/aws/services/memorydb/purchase_guard_test.go b/providers/aws/services/memorydb/purchase_guard_test.go new file mode 100644 index 00000000..e3b3a0ea --- /dev/null +++ b/providers/aws/services/memorydb/purchase_guard_test.go @@ -0,0 +1,137 @@ +package memorydb + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/memorydb" + "github.com/aws/aws-sdk-go-v2/service/memorydb/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// The purchaseguard rules (issue #10), asserted at the memorydb purchase path. Each +// mock is bounded with Test(t): an unexpected AWS call fails the test instead +// of panicking. + +func newGuardMock(t *testing.T) *MockMemoryDBClient { + t.Helper() + m := &MockMemoryDBClient{} + m.Test(t) + expectMDBOffering(m) + return m +} + +func guardClient(m *MockMemoryDBClient) *Client { return &Client{client: m, region: "eu-west-1"} } + +// No token: the guard never looks the reservation up and the purchase runs. +func TestPurchaseGuard_EmptyTokenSkipsLookup(t *testing.T) { + t.Parallel() + m := newGuardMock(t) + m.On("PurchaseReservedNodesOffering", mock.Anything, mock.Anything).Return(&memorydb.PurchaseReservedNodesOfferingOutput{ReservedNode: &types.ReservedNode{ReservationId: aws.String("rn-new")}}, nil).Once() + + result, err := guardClient(m).PurchaseCommitment(context.Background(), mdbIdemRec(), common.PurchaseOptions{}) + + require.NoError(t, err) + assert.True(t, result.Success) + assert.False(t, result.ExistingCommitment) + m.AssertNotCalled(t, "DescribeReservedNodes", mock.Anything, mock.Anything) + m.AssertNumberOfCalls(t, "PurchaseReservedNodesOffering", 1) +} + +// With a token, the guard looks up before buying, and the lookup runs strictly +// before the purchase call. +func TestPurchaseGuard_LookupPrecedesPurchase(t *testing.T) { + t.Parallel() + m := newGuardMock(t) + var order []string + m.On("DescribeReservedNodes", mock.Anything, mock.Anything). + Run(func(mock.Arguments) { order = append(order, "lookup") }). + Return(&memorydb.DescribeReservedNodesOutput{}, nil).Once() + m.On("PurchaseReservedNodesOffering", mock.Anything, mock.Anything). + Run(func(mock.Arguments) { order = append(order, "purchase") }). + Return(&memorydb.PurchaseReservedNodesOfferingOutput{ReservedNode: &types.ReservedNode{ReservationId: aws.String("rn-new")}}, nil).Once() + + _, err := guardClient(m).PurchaseCommitment(context.Background(), mdbIdemRec(), common.PurchaseOptions{IdempotencyToken: common.DeriveIdempotencyToken("exec-order", 0)}) + + require.NoError(t, err) + assert.Equal(t, []string{"lookup", "purchase"}, order) +} + +// A lookup failure refuses the purchase with the shared guard's error text. +func TestPurchaseGuard_LookupErrorRefusesWithLabel(t *testing.T) { + t.Parallel() + m := newGuardMock(t) + m.On("DescribeReservedNodes", mock.Anything, mock.Anything).Return((*memorydb.DescribeReservedNodesOutput)(nil), fmt.Errorf("access denied")).Once() + + _, err := guardClient(m).PurchaseCommitment(context.Background(), mdbIdemRec(), common.PurchaseOptions{IdempotencyToken: common.DeriveIdempotencyToken("exec-label", 0)}) + + require.Error(t, err) + assert.Contains(t, err.Error(), "idempotency lookup failed before MemoryDB reservation purchase (refusing to purchase to avoid a possible double-buy)") + m.AssertNotCalled(t, "PurchaseReservedNodesOffering", mock.Anything, mock.Anything) +} + +// AlreadyExists with a token but a failing re-lookup: the original purchase +// error is reported, never a false success. +func TestPurchaseGuard_AlreadyExistsRelookupErrorReportsPurchaseError(t *testing.T) { + t.Parallel() + m := newGuardMock(t) + m.On("DescribeReservedNodes", mock.Anything, mock.Anything).Return(&memorydb.DescribeReservedNodesOutput{}, nil).Once() + m.On("PurchaseReservedNodesOffering", mock.Anything, mock.Anything).Return((*memorydb.PurchaseReservedNodesOfferingOutput)(nil), &types.ReservedNodeAlreadyExistsFault{}).Once() + m.On("DescribeReservedNodes", mock.Anything, mock.Anything).Return((*memorydb.DescribeReservedNodesOutput)(nil), fmt.Errorf("throttled")).Once() + + result, err := guardClient(m).PurchaseCommitment(context.Background(), mdbIdemRec(), common.PurchaseOptions{IdempotencyToken: common.DeriveIdempotencyToken("exec-relookup", 0)}) + + require.Error(t, err) + var already *types.ReservedNodeAlreadyExistsFault + assert.True(t, errors.As(err, &already), "the original AlreadyExists error must be reported") + assert.False(t, result.Success) + assert.False(t, result.ExistingCommitment) +} + +// AlreadyExists with a token but a re-lookup that misses: same, no false success. +func TestPurchaseGuard_AlreadyExistsRelookupMissReportsPurchaseError(t *testing.T) { + t.Parallel() + m := newGuardMock(t) + m.On("DescribeReservedNodes", mock.Anything, mock.Anything).Return(&memorydb.DescribeReservedNodesOutput{}, nil).Twice() + m.On("PurchaseReservedNodesOffering", mock.Anything, mock.Anything).Return((*memorydb.PurchaseReservedNodesOfferingOutput)(nil), &types.ReservedNodeAlreadyExistsFault{}).Once() + + result, err := guardClient(m).PurchaseCommitment(context.Background(), mdbIdemRec(), common.PurchaseOptions{IdempotencyToken: common.DeriveIdempotencyToken("exec-miss", 0)}) + + require.Error(t, err) + assert.False(t, result.Success) + assert.False(t, result.ExistingCommitment) +} + +// No token: AlreadyExists is never recovered (and nothing is looked up). +func TestPurchaseGuard_AlreadyExistsWithoutTokenNotRecovered(t *testing.T) { + t.Parallel() + m := newGuardMock(t) + m.On("PurchaseReservedNodesOffering", mock.Anything, mock.Anything).Return((*memorydb.PurchaseReservedNodesOfferingOutput)(nil), &types.ReservedNodeAlreadyExistsFault{}).Once() + + result, err := guardClient(m).PurchaseCommitment(context.Background(), mdbIdemRec(), common.PurchaseOptions{}) + + require.Error(t, err) + assert.False(t, result.Success) + assert.False(t, result.ExistingCommitment) + m.AssertNotCalled(t, "DescribeReservedNodes", mock.Anything, mock.Anything) +} + +// A purchase error that is not AlreadyExists triggers no recovery lookup. +func TestPurchaseGuard_OtherPurchaseErrorNotRecovered(t *testing.T) { + t.Parallel() + m := newGuardMock(t) + m.On("DescribeReservedNodes", mock.Anything, mock.Anything).Return(&memorydb.DescribeReservedNodesOutput{}, nil).Once() + m.On("PurchaseReservedNodesOffering", mock.Anything, mock.Anything).Return((*memorydb.PurchaseReservedNodesOfferingOutput)(nil), fmt.Errorf("boom")).Once() + + result, err := guardClient(m).PurchaseCommitment(context.Background(), mdbIdemRec(), common.PurchaseOptions{IdempotencyToken: common.DeriveIdempotencyToken("exec-other", 0)}) + + require.Error(t, err) + assert.False(t, result.Success) + m.AssertNumberOfCalls(t, "DescribeReservedNodes", 1) +}