diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index 78ad4e869..cbf71e610 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -44,6 +44,7 @@ var serverNotePatterns = []string{ regexp.QuoteMeta(redshiftRecentBuyRefusal), regexp.QuoteMeta(redshiftRecentBuyRefusalLegacy), regexp.QuoteMeta(redshiftSettleRefusal), + regexp.QuoteMeta(redshiftDuplicateTupleRefusal), regexp.QuoteMeta(gcpDuplicateCheckFailedNote), regexp.QuoteMeta(gcpRecentCommitmentNote), regexp.QuoteMeta(elastiCacheDuplicateCheckFailedNote), diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go index a6debd96c..8d66ea704 100644 --- a/internal/purchase/errnotes_test.go +++ b/internal/purchase/errnotes_test.go @@ -64,6 +64,7 @@ func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { redshiftRecentBuyRefusal, redshiftRecentBuyRefusalLegacy, redshiftSettleRefusal, + redshiftDuplicateTupleRefusal, gcpDuplicateCheckFailedNote, gcpRecentCommitmentNote, elastiCacheDuplicateCheckFailedNote, @@ -220,7 +221,7 @@ func TestSanitizeStoredError_ResolveNoteExchangeIDIsClosed(t *testing.T) { // The Redshift gate texts are fixed: an appended execution id or raw text must // not pass verbatim. func TestSanitizeStoredError_RedshiftGateRefusalsAreClosed(t *testing.T) { - for _, text := range []string{redshiftConflictRefusal, redshiftHorizonRefusal, redshiftNoStampRefusal, redshiftFailedConflictRefusal, redshiftNoJournalRefusal, redshiftRecentBuyRefusal, redshiftSettleRefusal, gcpDuplicateCheckFailedNote, gcpRecentCommitmentNote, elastiCacheDuplicateCheckFailedNote, elastiCacheRecentReservationNote} { + for _, text := range []string{redshiftConflictRefusal, redshiftHorizonRefusal, redshiftNoStampRefusal, redshiftFailedConflictRefusal, redshiftNoJournalRefusal, redshiftRecentBuyRefusal, redshiftSettleRefusal, redshiftDuplicateTupleRefusal, gcpDuplicateCheckFailedNote, gcpRecentCommitmentNote, elastiCacheDuplicateCheckFailedNote, elastiCacheRecentReservationNote} { assert.Equal(t, text, SanitizeStoredError(text)) for _, variant := range []string{ text + " (execution 00000000-0000-0000-0000-000000000000, running)", diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index 5c3162cb9..3ee3f2a2f 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -435,6 +435,9 @@ func (m *Manager) preExecutionRefusal(ctx context.Context, exec *config.Purchase if refusal := m.ladderExecutionRefusal(ctx, exec); refusal != nil { return beforeSubmission(refusal) } + if hasDuplicateRedshiftTuple(exec) { + return beforeSubmission(errors.New(redshiftDuplicateTupleRefusal)) + } // A Redshift retry reuses the first attempt's request time; only the journal lets go's dedupe tell its node from another's. if hasRedshiftRec(exec) && exec.RetryAttemptN > 0 && m.attemptJournal == nil { return errors.New(redshiftNoJournalRefusal) diff --git a/internal/purchase/redshift_duplicate_tuple_test.go b/internal/purchase/redshift_duplicate_tuple_test.go new file mode 100644 index 000000000..d864312a2 --- /dev/null +++ b/internal/purchase/redshift_duplicate_tuple_test.go @@ -0,0 +1,95 @@ +package purchase + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func dupTupleExec(recs ...config.RecommendationRecord) *config.PurchaseExecution { + return &config.PurchaseExecution{ + ExecutionID: "rs-dup", IdempotencyKey: "rs-dup-lineage", Status: "running", Recommendations: recs, + } +} + +func TestRedshiftDuplicateTuple_RefusedBeforeSubmission(t *testing.T) { + at := time.Now().UTC().Add(-time.Minute) + exec := dupTupleExec(redshiftScopedRec("acct-A"), redshiftScopedRec("acct-A")) + run := runRedshiftFinalize(t, exec, nil, common.ServiceRedshift, stampAt(at)) + + require.Error(t, run.err) + assert.Equal(t, redshiftDuplicateTupleRefusal, run.err.Error()) + assert.True(t, isBeforeSubmission(run.err)) + assert.True(t, exec.NoSubmission, "nothing was submitted, so the row is retryable from scratch") + assert.Equal(t, "failed", exec.Status) + assert.Empty(t, run.opts, "no PurchaseCommitment call") + assert.Empty(t, run.stampedIDs, "refused before the dispatch gate stamps") +} + +func TestRedshiftDuplicateTuple_Detection(t *testing.T) { + a := redshiftScopedRec("acct-A") + diff := func(f func(*config.RecommendationRecord)) config.RecommendationRecord { + r := redshiftScopedRec("acct-A") + f(&r) + return r + } + unselected := redshiftScopedRec("acct-A") + unselected.Selected = false + ec2 := scopedTestRec("acct-A") + noAcct := redshiftScopedRec("acct-A") + noAcct.CloudAccountID = nil + + cases := map[string]struct { + recs []config.RecommendationRecord + refused bool + }{ + "identical tuple and account": {[]config.RecommendationRecord{a, a}, true}, + "identical, third one differs": {[]config.RecommendationRecord{a, diff(func(r *config.RecommendationRecord) { r.Region = "eu-west-1" }), a}, true}, + "different node type": {[]config.RecommendationRecord{a, diff(func(r *config.RecommendationRecord) { r.ResourceType = "ra3.4xlarge" })}, false}, + "different region": {[]config.RecommendationRecord{a, diff(func(r *config.RecommendationRecord) { r.Region = "eu-west-1" })}, false}, + "different term": {[]config.RecommendationRecord{a, diff(func(r *config.RecommendationRecord) { r.Term = 3 })}, false}, + "different payment": {[]config.RecommendationRecord{a, diff(func(r *config.RecommendationRecord) { r.Payment = "no_upfront" })}, false}, + "different count": {[]config.RecommendationRecord{a, diff(func(r *config.RecommendationRecord) { r.Count = 2 })}, false}, + "different account": {[]config.RecommendationRecord{a, redshiftScopedRec("acct-B")}, false}, + "duplicate is not selected": {[]config.RecommendationRecord{a, unselected}, false}, + "non-redshift twins": {[]config.RecommendationRecord{ec2, ec2}, false}, + "single rec": {[]config.RecommendationRecord{a}, false}, + "both without an account": {[]config.RecommendationRecord{noAcct, noAcct}, true}, + "one scoped, one without scope": {[]config.RecommendationRecord{a, noAcct}, false}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + m := &Manager{config: new(MockConfigStore), attemptJournal: &fakeAttemptJournal{}} + err := m.preExecutionRefusal(context.Background(), dupTupleExec(tc.recs...)) + if tc.refused { + require.Error(t, err) + assert.Equal(t, redshiftDuplicateTupleRefusal, err.Error()) + assert.True(t, isBeforeSubmission(err)) + return + } + assert.NoError(t, err) + }) + } +} + +// A rec with no account of its own buys in the execution's account, so it duplicates a scoped twin there. +func TestRedshiftDuplicateTuple_UnscopedRecUsesTheExecutionAccount(t *testing.T) { + acct := "acct-A" + noAcct := redshiftScopedRec("acct-A") + noAcct.CloudAccountID = nil + exec := dupTupleExec(redshiftScopedRec("acct-A"), noAcct) + exec.CloudAccountID = &acct + + err := (&Manager{config: new(MockConfigStore)}).preExecutionRefusal(context.Background(), exec) + require.Error(t, err) + assert.Equal(t, redshiftDuplicateTupleRefusal, err.Error()) + + other := "acct-B" + exec.CloudAccountID = &other + assert.NoError(t, (&Manager{config: new(MockConfigStore)}).preExecutionRefusal(context.Background(), exec)) +} diff --git a/internal/purchase/redshift_lineage.go b/internal/purchase/redshift_lineage.go index dce8e3693..16e03ad0b 100644 --- a/internal/purchase/redshift_lineage.go +++ b/internal/purchase/redshift_lineage.go @@ -36,6 +36,32 @@ func hasRedshiftRec(exec *config.PurchaseExecution) bool { return false } +// hasDuplicateRedshiftTuple reports whether two selected Redshift recs share a tuple and account. The +// go client adopts a node it finds for the same offering and count, so the second would buy nothing. +func hasDuplicateRedshiftTuple(exec *config.PurchaseExecution) bool { + type key struct { + tuple config.RedshiftTuple + account string + } + seen := map[key]bool{} + for _, t := range redshiftTuples(exec) { + account := "" + switch { + case t.CloudAccountID != nil: + account = *t.CloudAccountID + case exec.CloudAccountID != nil: + account = *exec.CloudAccountID + } + t.CloudAccountID = nil + k := key{t, account} + if seen[k] { + return true + } + seen[k] = true + } + return false +} + // afterRedshiftStampHook runs inside the gate transaction right after the // stamp. Tests set it to observe the lock wait; it is nil in production. var afterRedshiftStampHook func() @@ -51,6 +77,7 @@ const ( redshiftRecentBuyRefusal = "refusing to dispatch: another purchase of the same Redshift reservation (region, node type, term, payment, count) ended too close to this purchase's first dispatch for AWS to tell the two apart, so start a new purchase at least 10 minutes after the other one ended, or, if it failed and can still be retried, retry or settle it" // redshiftRecentBuyRefusalLegacy is the text stored before failed rows joined the adoption window. redshiftRecentBuyRefusalLegacy = "refusing to dispatch: another purchase of the same Redshift reservation (region, node type, term, payment, count) completed too close to this purchase's first dispatch for AWS to tell the two apart, so start a new purchase at least 10 minutes after the other one completed" + redshiftDuplicateTupleRefusal = "refusing to dispatch: this purchase selects the same Redshift reservation (region, node type, term, payment, count) twice for one account, and AWS would treat the second as the first, so keep one of them in the selection and start a new purchase" redshiftSettleRefusal = "refusing to dispatch: another attempt of this Redshift purchase ended less than 10 minutes ago and may have bought, so wait 10 minutes and then retry" ) diff --git a/known_issues/39_redshift_dedupe_window.md b/known_issues/39_redshift_dedupe_window.md index 8fa3c95a6..8c42b6558 100644 --- a/known_issues/39_redshift_dedupe_window.md +++ b/known_issues/39_redshift_dedupe_window.md @@ -12,10 +12,11 @@ **Mitigation (platform-recorded buys):** The Redshift dispatch gate now refuses a purchase when another lineage's completed, partially completed or revocation-requested row, or a failed row without the no-submission proof (settled or not, and whether or not Retry is refused), for the same tuple was last saved within 10 minutes of the new lineage's first dispatch time. The refusal is recorded before submission with a fixed text that names no execution, and the user starts a new purchase at least 10 minutes after the other one ended, or retries or settles it if it failed and can still be retried. +**Two identical-tuple recs in one execution** (same region, node type, term, payment, count and account) are refused before submission with a fixed text (`redshiftDuplicateTupleRefusal`, checked in `preExecutionRefusal`), since the go client would adopt the first node for the second. The user keeps one of them and starts a new purchase. + **Remaining:** - Buys made outside the platform are invisible to the gate. -- Two identical-tuple recs in one execution still adopt each other (platform #1809). - A lineage whose purchase record never landed (the child bought but its row save failed, `execution.go:351` and `:383`) leaves no completed row for the gate to see. - The go-side narrowing in LeanerCloud/cloud-commitments-go#700 and the real fix in #515 are open. - The window is measured from the other row's last save, so a later save of that row (for example a revocation request) restarts it. That only refuses longer; it never allows a double buy.