diff --git a/internal/config/store_postgres_redshift_lineage.go b/internal/config/store_postgres_redshift_lineage.go index 5e358ad5f..53ceadb44 100644 --- a/internal/config/store_postgres_redshift_lineage.go +++ b/internal/config/store_postgres_redshift_lineage.go @@ -71,6 +71,16 @@ func redshiftFailedClause(settleReady bool) string { AND e.first_dispatch_at > now() - make_interval(secs => $3)` + notSettled + `)` } +// redshiftRecentBuyClause matches other-family rows that may have bought (finished, or failed +// without the no-submission proof, settled or not) and were last written within $7 seconds of +// the dispatch stamp $6: go could adopt their node. It is also the recent_buy column. +func redshiftRecentBuyClause() string { + return `(split_part(COALESCE(NULLIF(e.idempotency_key, ''), e.execution_id::text), ':', 1) <> $2 + AND e.updated_at >= $6::timestamptz - make_interval(secs => $7) + AND (e.status IN ('completed', 'partially_completed', 'revocation_requested') + OR (e.status = 'failed' AND NOT e.no_submission)))` +} + // redshiftSameFamilySettleClause matches a same-family row whose own attempt may have bought and // was last written within $8 seconds: its node can still be missing from AWS describe results. // A linked failed row already passed the Retry settle (and Retry bumps its updated_at). @@ -83,12 +93,14 @@ func redshiftSameFamilySettleClause() string { // RedshiftLineageConflict is an open execution whose Redshift rec matches a // candidate tuple. SameFamily is true when it shares the candidate's lineage -// root (idempotency key up to the first colon). +// root (idempotency key up to the first colon). RecentBuy marks an other-family row that may +// have bought and was written inside the adoption window. type RedshiftLineageConflict struct { ExecutionID string Status string FirstDispatchAt *time.Time SameFamily bool + RecentBuy bool Error string Recommendations []RecommendationRecord } @@ -96,7 +108,8 @@ type RedshiftLineageConflict struct { // FindOpenRedshiftLineageConflictsTx returns the other executions whose node this lineage could // buy twice or adopt. Account: row's else rec's vs execution's else tuple's; NULL matches any. // Running rows conflict at any age; other open rows within horizon of their stamp. Other-family -// finished rows within window of stamp; other-family retryable failed rows within horizon. +// rows that may have bought (finished, or failed without the no-submission proof) within window +// of stamp; other-family retryable failed rows within horizon. // Same-family rows: running, or failed (unlinked) or finished after a possible send, within settle. func (s *PostgresStore) FindOpenRedshiftLineageConflictsTx(ctx context.Context, tx pgx.Tx, selfID, family string, tuples []RedshiftTuple, accountID *string, horizon time.Duration, stamp time.Time, window, settle time.Duration) ([]RedshiftLineageConflict, error) { if family == "" { @@ -116,7 +129,8 @@ func (s *PostgresStore) FindOpenRedshiftLineageConflictsTx(ctx context.Context, } rows, err := tx.Query(ctx, ` SELECT e.execution_id, e.status, e.first_dispatch_at, COALESCE(e.error, ''), e.recommendations, - split_part(COALESCE(NULLIF(e.idempotency_key, ''), e.execution_id::text), ':', 1) = $2 + split_part(COALESCE(NULLIF(e.idempotency_key, ''), e.execution_id::text), ':', 1) = $2, + `+redshiftRecentBuyClause()+` FROM purchase_executions e WHERE e.execution_id <> $1 AND e.first_dispatch_at IS NOT NULL @@ -125,9 +139,7 @@ func (s *PostgresStore) FindOpenRedshiftLineageConflictsTx(ctx context.Context, OR (split_part(COALESCE(NULLIF(e.idempotency_key, ''), e.execution_id::text), ':', 1) <> $2 AND e.status IN ('approved', 'scheduled', 'pending', 'notified', 'paused') AND e.first_dispatch_at > now() - make_interval(secs => $3)) - OR (split_part(COALESCE(NULLIF(e.idempotency_key, ''), e.execution_id::text), ':', 1) <> $2 - AND e.status IN ('completed', 'partially_completed', 'revocation_requested') - AND e.updated_at >= $6::timestamptz - make_interval(secs => $7)) + OR `+redshiftRecentBuyClause()+` OR `+redshiftFailedClause(settleReady)+` OR `+redshiftSameFamilySettleClause()+` ) @@ -154,7 +166,7 @@ func (s *PostgresStore) FindOpenRedshiftLineageConflictsTx(ctx context.Context, for rows.Next() { var c RedshiftLineageConflict var recsJSON []byte - if err := rows.Scan(&c.ExecutionID, &c.Status, &c.FirstDispatchAt, &c.Error, &recsJSON, &c.SameFamily); err != nil { + if err := rows.Scan(&c.ExecutionID, &c.Status, &c.FirstDispatchAt, &c.Error, &recsJSON, &c.SameFamily, &c.RecentBuy); err != nil { return nil, fmt.Errorf("scan redshift lineage conflict: %w", err) } if err := json.Unmarshal(recsJSON, &c.Recommendations); err != nil { diff --git a/internal/config/store_postgres_redshift_lineage_integration_test.go b/internal/config/store_postgres_redshift_lineage_integration_test.go index 478de9300..cab682da8 100644 --- a/internal/config/store_postgres_redshift_lineage_integration_test.go +++ b/internal/config/store_postgres_redshift_lineage_integration_test.go @@ -44,11 +44,17 @@ func seedLineageRow(ctx context.Context, t *testing.T, store *PostgresStore, fam } func findLineageConflicts(ctx context.Context, t *testing.T, store *PostgresStore, self, family string) []RedshiftLineageConflict { + t.Helper() + return findLineageConflictsAt(ctx, t, store, self, family, time.Now()) +} + +// findLineageConflictsAt runs the query with an explicit dispatch stamp. +func findLineageConflictsAt(ctx context.Context, t *testing.T, store *PostgresStore, self, family string, stamp time.Time) []RedshiftLineageConflict { t.Helper() var got []RedshiftLineageConflict require.NoError(t, store.WithTx(ctx, func(tx pgx.Tx) error { var err error - got, err = store.FindOpenRedshiftLineageConflictsTx(ctx, tx, self, family, []RedshiftTuple{lineageTuple}, nil, lineageTestHorizon, time.Now(), lineageTestWindow, lineageTestSettle) + got, err = store.FindOpenRedshiftLineageConflictsTx(ctx, tx, self, family, []RedshiftTuple{lineageTuple}, nil, lineageTestHorizon, stamp, lineageTestWindow, lineageTestSettle) return err })) return got @@ -398,6 +404,7 @@ func TestFindOpenRedshiftLineageConflicts_AdoptionWindow(t *testing.T) { assert.Equal(t, other.ExecutionID, got[0].ExecutionID) assert.Equal(t, "completed", got[0].Status) assert.False(t, got[0].SameFamily) + assert.True(t, got[0].RecentBuy) }) t.Run("partially_completed_conflicts", func(t *testing.T) { reset(t) @@ -505,6 +512,8 @@ func TestFindOpenRedshiftLineageConflicts_FailedRows(t *testing.T) { store := setupLadderStore(ctx, t) recent, old := ptrDur(time.Hour), ptrDur(48*time.Hour) // seedFailed seeds an other-family failed row and applies a column update to it. + // pastWindow is a stamp whose adoption window ended before the rows just written. + pastWindow := func() time.Time { return time.Now().Add(lineageTestWindow + time.Hour) } seedFailed := func(t *testing.T, family string, stampAge *time.Duration, set string) (self, failed *PurchaseExecution) { t.Helper() _, err := store.db.Exec(ctx, `DELETE FROM purchase_executions`) @@ -524,7 +533,9 @@ func TestFindOpenRedshiftLineageConflicts_FailedRows(t *testing.T) { t.Run("failed_unlinked_conflicts", func(t *testing.T) { self, failed := seedFailed(t, "fam-f1", recent, "") - assert.Equal(t, []string{failed.ExecutionID}, conflictIDs(findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0"))) + got := findLineageConflictsAt(ctx, t, store, self.ExecutionID, "fam-f0", pastWindow()) + assert.Equal(t, []string{failed.ExecutionID}, conflictIDs(got)) + assert.False(t, got[0].RecentBuy, "the row comes from the failed clause") }) t.Run("failed_attempt1_no_submission_conflicts", func(t *testing.T) { self, failed := seedFailed(t, "fam-f2", recent, "no_submission = true, retry_attempt_n = 1") @@ -532,19 +543,25 @@ func TestFindOpenRedshiftLineageConflicts_FailedRows(t *testing.T) { }) t.Run("failed_linked_excluded", func(t *testing.T) { self, _ := seedFailed(t, "fam-f3", recent, "retry_execution_id = $2") - assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0")) + assert.Empty(t, findLineageConflictsAt(ctx, t, store, self.ExecutionID, "fam-f0", pastWindow())) }) t.Run("failed_attempt0_no_submission_excluded", func(t *testing.T) { self, _ := seedFailed(t, "fam-f4", recent, "no_submission = true") - assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0")) + assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0"), "no recent buy either: the no-submission proof") }) t.Run("failed_older_than_H_excluded", func(t *testing.T) { self, _ := seedFailed(t, "fam-f5", old, "") - assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0")) + assert.Empty(t, findLineageConflictsAt(ctx, t, store, self.ExecutionID, "fam-f0", pastWindow())) }) t.Run("failed_settled_excluded", func(t *testing.T) { self, _ := seedFailed(t, "fam-f6", recent, "outcome_settled_at = now(), outcome_settled_by = 'op', outcome_settlement = 'settled'") - assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0")) + assert.Empty(t, findLineageConflictsAt(ctx, t, store, self.ExecutionID, "fam-f0", pastWindow())) + }) + t.Run("failed_settled_inside_adoption_window_conflicts_as_recent_buy", func(t *testing.T) { + self, failed := seedFailed(t, "fam-f6b", recent, "outcome_settled_at = now(), outcome_settled_by = 'op', outcome_settlement = 'settled'") + got := findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0") + assert.Equal(t, []string{failed.ExecutionID}, conflictIDs(got)) + assert.True(t, got[0].RecentBuy) }) t.Run("failed_settled_still_conflicts_before_the_settlement_migration", func(t *testing.T) { // Prod may run this code before migration 000155: the settled filter must drop out, not error. @@ -558,7 +575,9 @@ func TestFindOpenRedshiftLineageConflicts_FailedRows(t *testing.T) { }) _, err := store.db.Exec(ctx, `UPDATE schema_migrations SET version = $1, dirty = false`, settlementMigrationVersion-1) require.NoError(t, err) - assert.Equal(t, []string{failed.ExecutionID}, conflictIDs(findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0"))) + got := findLineageConflictsAt(ctx, t, store, self.ExecutionID, "fam-f0", pastWindow()) + assert.Equal(t, []string{failed.ExecutionID}, conflictIDs(got)) + assert.False(t, got[0].RecentBuy, "the row comes from the failed clause") }) t.Run("same_family_failed_inside_settle_conflicts", func(t *testing.T) { self, failed := seedFailed(t, "fam-f0:acct", recent, "") @@ -587,15 +606,17 @@ func TestFindOpenRedshiftLineageConflicts_FailedRows(t *testing.T) { self, _ := seedFailed(t, "fam-f0:acct", recent, "no_submission = true, retry_attempt_n = 1") assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0")) }) - t.Run("same_family_completed_inside_settle_conflicts", func(t *testing.T) { - _, err := store.db.Exec(ctx, `DELETE FROM purchase_executions`) - require.NoError(t, err) - self := seedLineageRow(ctx, t, store, "fam-f0", "running", recent) - done := seedLineageRow(ctx, t, store, "fam-f0:acct", "completed", recent) - got := findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0") - assert.Equal(t, []string{done.ExecutionID}, conflictIDs(got)) - assert.True(t, got[0].SameFamily) - }) + for _, status := range []string{"completed", "partially_completed", "revocation_requested"} { + t.Run("same_family_"+status+"_inside_settle_conflicts", func(t *testing.T) { + _, err := store.db.Exec(ctx, `DELETE FROM purchase_executions`) + require.NoError(t, err) + self := seedLineageRow(ctx, t, store, "fam-f0", "running", recent) + done := seedLineageRow(ctx, t, store, "fam-f0:acct", status, recent) + got := findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0") + assert.Equal(t, []string{done.ExecutionID}, conflictIDs(got)) + assert.True(t, got[0].SameFamily) + }) + } t.Run("same_family_completed_after_settle_passes", func(t *testing.T) { _, err := store.db.Exec(ctx, `DELETE FROM purchase_executions`) require.NoError(t, err) @@ -617,3 +638,56 @@ func TestFindOpenRedshiftLineageConflicts_FailedRows(t *testing.T) { assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0")) }) } + +func TestFindOpenRedshiftLineageConflicts_RecentBuyKinds(t *testing.T) { + ctx := context.Background() + store := setupLadderStore(ctx, t) + recent := ptrDur(time.Hour) + settled := "outcome_settled_at = now(), outcome_settled_by = 'op', outcome_settlement = 'settled'" + for _, tc := range []struct { + name, status, set string + recentBuy bool + }{ + {"completed", "completed", "", true}, + {"partially_completed", "partially_completed", "", true}, + {"revocation_requested", "revocation_requested", "", true}, + {"failed_unsettled", "failed", "", true}, + {"failed_settled", "failed", settled, true}, + {"failed_retry_linked", "failed", "retry_execution_id = $2", true}, + {"failed_no_submission", "failed", "no_submission = true", false}, + {"failed_no_submission_attempt1", "failed", "no_submission = true, retry_attempt_n = 1", false}, + } { + t.Run(tc.name, func(t *testing.T) { + _, err := store.db.Exec(ctx, `DELETE FROM purchase_executions`) + require.NoError(t, err) + self := seedLineageRow(ctx, t, store, "fam-rb", "running", recent) + other := seedLineageRow(ctx, t, store, "fam-rb-other", tc.status, recent) + if tc.set != "" { + args := []any{other.ExecutionID} + if strings.Contains(tc.set, "$2") { + args = append(args, self.ExecutionID) + } + _, err = store.db.Exec(ctx, `UPDATE purchase_executions SET `+tc.set+` WHERE execution_id = $1`, args...) + require.NoError(t, err) + } + var at time.Time + require.NoError(t, store.db.QueryRow(ctx, `SELECT updated_at FROM purchase_executions WHERE execution_id = $1`, other.ExecutionID).Scan(&at)) + + var inside RedshiftLineageConflict + for _, c := range findLineageConflictsAt(ctx, t, store, self.ExecutionID, "fam-rb", at) { + if c.ExecutionID == other.ExecutionID { + inside = c + } + } + assert.Equal(t, tc.recentBuy, inside.RecentBuy, "inside the window") + if tc.recentBuy { + assert.Equal(t, other.ExecutionID, inside.ExecutionID, "inside the window") + } + + // One microsecond past the window edge the row is no longer a recent buy. + for _, c := range findLineageConflictsAt(ctx, t, store, self.ExecutionID, "fam-rb", at.Add(lineageTestWindow+time.Microsecond)) { + assert.False(t, c.RecentBuy, "outside the window") + } + }) + } +} diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index e3a99959d..048ddaebc 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -41,6 +41,7 @@ var serverNotePatterns = []string{ regexp.QuoteMeta(redshiftFailedConflictRefusal), regexp.QuoteMeta(redshiftNoJournalRefusal), regexp.QuoteMeta(redshiftRecentBuyRefusal), + regexp.QuoteMeta(redshiftRecentBuyRefusalLegacy), regexp.QuoteMeta(redshiftSettleRefusal), `EC2 recommendation for ` + resourceTypePattern + ` has zonal scope but no AvailabilityZone: refusing to buy a zonal RI in an arbitrary AZ; re-fetch the recommendation`, `zonal RI offering needs an availability zone; re-fetch the recommendation`, diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go index 88f6b68f7..8dc1fdc59 100644 --- a/internal/purchase/errnotes_test.go +++ b/internal/purchase/errnotes_test.go @@ -60,6 +60,7 @@ func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { redshiftFailedConflictRefusal, redshiftNoJournalRefusal, redshiftRecentBuyRefusal, + redshiftRecentBuyRefusalLegacy, redshiftSettleRefusal, "EC2 recommendation for m5.large has zonal scope but no AvailabilityZone: refusing to buy a zonal RI in an arbitrary AZ; re-fetch the recommendation", "zonal RI offering needs an availability zone; re-fetch the recommendation", diff --git a/internal/purchase/idempotency_key_writers_test.go b/internal/purchase/idempotency_key_writers_test.go index 497fbc55d..be49fc094 100644 --- a/internal/purchase/idempotency_key_writers_test.go +++ b/internal/purchase/idempotency_key_writers_test.go @@ -101,6 +101,7 @@ var ( upsertSet = regexp.MustCompile(`(?is)\bDO\s+UPDATE\s+SET\b`) setTerminator = regexp.MustCompile(`(?i)^(WHERE|RETURNING)\b`) idempotencyKeyColumn = regexp.MustCompile(`(?i)\bidempotency_key\b`) + keyAssignmentTarget = regexp.MustCompile(`(?i)^"?idempotency_key"?\s*=`) ) // columnList returns the balanced "(...)" after an INSERT target and the text @@ -121,11 +122,14 @@ func columnList(rest string) (cols, tail string, ok bool) { } // setText returns the SET clause text in rest, cut at the first WHERE, RETURNING or ";" -// outside parentheses, so a subselect's WHERE does not hide a later assignment. +// outside parentheses and '...' literals, so a subselect's WHERE does not hide a later assignment. func setText(rest string) string { - depth := 0 + depth, inQuote := 0, false for i, r := range rest { switch { + case r == '\'': + inQuote = !inQuote + case inQuote: case r == '(': depth++ case r == ')': @@ -143,8 +147,49 @@ func isWordByte(b byte) bool { return b == '_' || b >= '0' && b <= '9' || b >= 'a' && b <= 'z' || b >= 'A' && b <= 'Z' } +// setItems splits a SET clause at commas outside parentheses and '...' literals. +// It does not parse -- comments, $$...$$ or E'...' strings; no SQL in this repo uses them in a SET. +func setItems(set string) []string { + var items []string + depth, start, inQuote := 0, 0, false + for i, r := range set { + switch { + case r == '\'': + inQuote = !inQuote + case inQuote: + case r == '(': + depth++ + case r == ')': + depth-- + case r == ',' && depth == 0: + items = append(items, set[start:i]) + start = i + 1 + } + } + return append(items, set[start:]) +} + +// setsIdempotencyKey reports whether a SET clause assigns the column: as a plain target, or +// inside the column list of a row assignment. A reference in a value or subquery does not count. +func setsIdempotencyKey(set string) bool { + for _, item := range setItems(set) { + item = strings.TrimSpace(item) + if !strings.HasPrefix(item, "(") { + if keyAssignmentTarget.MatchString(item) { + return true + } + continue + } + cols, tail, ok := columnList(item) + if ok && strings.HasPrefix(strings.TrimSpace(tail), "=") && idempotencyKeyColumn.MatchString(cols) { + return true + } + } + return false +} + // sqlIdempotencyKeyWriters counts purchase_executions INSERTs carrying idempotency_key -// (no column list counts) and UPDATEs or upserts that SET it; dynamic counts unverifiable INSERTs. +// (no column list counts) and UPDATEs or upserts that assign it in SET; dynamic counts unverifiable INSERTs. func sqlIdempotencyKeyWriters(sql string) (inserts, updates, dynamic int) { for _, loc := range insertPurchaseExecutions.FindAllStringIndex(sql, -1) { rest := strings.TrimSpace(sql[loc[1]:]) @@ -164,13 +209,13 @@ func sqlIdempotencyKeyWriters(sql string) (inserts, updates, dynamic int) { tail = tail[:end] } for _, loc := range upsertSet.FindAllStringIndex(tail, -1) { - if idempotencyKeyColumn.MatchString(setText(tail[loc[1]:])) { + if setsIdempotencyKey(setText(tail[loc[1]:])) { updates++ } } } for _, loc := range updatePurchaseExecutions.FindAllStringIndex(sql, -1) { - if idempotencyKeyColumn.MatchString(setText(sql[loc[1]:])) { + if setsIdempotencyKey(setText(sql[loc[1]:])) { updates++ } } @@ -334,6 +379,17 @@ func TestIdempotencyKeySQLScan_Detects(t *testing.T) { {"update key set after a FROM inside parentheses", `UPDATE purchase_executions SET status = (SELECT s FROM t), idempotency_key = $2 WHERE execution_id = $1`, 0, 1, 0}, {"update key set after a WHERE inside parentheses", `UPDATE purchase_executions SET x = (SELECT a FROM t WHERE b = 1), idempotency_key = $2 WHERE id = $1`, 0, 1, 0}, {"upsert key set after a WHERE inside parentheses", `INSERT INTO purchase_executions (execution_id) VALUES ($1) ON CONFLICT (execution_id) DO UPDATE SET x = (SELECT a FROM t WHERE b = 1), idempotency_key = $2`, 0, 1, 0}, + {"update filters on the key inside a SET subquery", `UPDATE purchase_executions SET a = (SELECT x FROM t WHERE idempotency_key = $1) WHERE execution_id = $2`, 0, 0, 0}, + {"update sets the key after a subquery that filters on it", `UPDATE purchase_executions SET a = (SELECT x FROM t WHERE idempotency_key = $1), idempotency_key = $2 WHERE execution_id = $3`, 0, 1, 0}, + {"update row assignment names the key", `UPDATE purchase_executions SET (status, idempotency_key) = ($1, $2) WHERE execution_id = $3`, 0, 1, 0}, + {"update row assignment without the key", `UPDATE purchase_executions SET (status, error) = (SELECT s, idempotency_key FROM t) WHERE execution_id = $1`, 0, 0, 0}, + {"update quoted identifier target", `UPDATE purchase_executions SET "idempotency_key" = $1 WHERE execution_id = $2`, 0, 1, 0}, + {"update target without space before equals", `UPDATE purchase_executions SET IDEMPOTENCY_KEY=$1 WHERE execution_id = $2`, 0, 1, 0}, + {"update literal with a paren and comma before the key target", `UPDATE purchase_executions SET error = 'x (', idempotency_key = $2 WHERE execution_id = $1`, 0, 1, 0}, + {"update literal that mentions WHERE before the key target", `UPDATE purchase_executions SET error = 'see WHERE', idempotency_key = $1 WHERE execution_id = $2`, 0, 1, 0}, + {"update literal that mentions the key", `UPDATE purchase_executions SET error = 'idempotency_key = 1, y' WHERE execution_id = $1`, 0, 0, 0}, + {"update with the key set twice counts once", `UPDATE purchase_executions SET idempotency_key = $1, idempotency_key = $2`, 0, 1, 0}, + {"upsert filters on the key inside a SET subquery", `INSERT INTO purchase_executions (execution_id) VALUES ($1) ON CONFLICT (execution_id) DO UPDATE SET a = (SELECT x FROM t WHERE idempotency_key = $2)`, 0, 0, 0}, {"insert column list built with Sprintf", `INSERT INTO purchase_executions (%s) VALUES (%s)`, 0, 0, 1}, {"insert column list unclosed by concatenation", `INSERT INTO purchase_executions (execution_id, `, 0, 0, 1}, {"insert column list empty before concatenation", `INSERT INTO purchase_executions (`, 0, 0, 1}, diff --git a/internal/purchase/redshift_gate_test.go b/internal/purchase/redshift_gate_test.go index 5009e42d7..c7510bc98 100644 --- a/internal/purchase/redshift_gate_test.go +++ b/internal/purchase/redshift_gate_test.go @@ -83,7 +83,7 @@ func TestRedshiftDispatchGate_RecentFinishedBuyRefusedWithFixedText(t *testing.T for _, status := range []string{config.StatusCompleted, config.StatusPartiallyCompleted, config.StatusRevocationRequested} { exec := gateExec(0, gateRec("acct-A", 2)) stamp := stamped() - run := runGate(t, exec, stamp, []config.RedshiftLineageConflict{{ExecutionID: "other-id", Status: status}}) + run := runGate(t, exec, stamp, []config.RedshiftLineageConflict{{ExecutionID: "other-id", Status: status, RecentBuy: true}}) require.Error(t, run.err, status) assert.True(t, isBeforeSubmission(run.err), status) @@ -97,7 +97,7 @@ func TestRedshiftDispatchGate_RecentFinishedBuyRefusedWithFixedText(t *testing.T func TestRedshiftDispatchGate_FinishedConflictWinsOverOpenOne(t *testing.T) { run := runGate(t, gateExec(0, gateRec("acct-A", 2)), stamped(), []config.RedshiftLineageConflict{ - {ExecutionID: "open", Status: "running"}, {ExecutionID: "done", Status: config.StatusCompleted}}) + {ExecutionID: "open", Status: "running"}, {ExecutionID: "done", Status: config.StatusCompleted, RecentBuy: true}}) require.Error(t, run.err) assert.Equal(t, redshiftRecentBuyRefusal, run.err.Error()) @@ -115,7 +115,7 @@ func TestRedshiftDispatchGate_SameFamilyRecentFailureRefusesWithSettleText(t *te assert.Nil(t, exec.FirstDispatchAt, "a refused dispatch records no stamp") run = runGate(t, gateExec(1, gateRec("acct-A", 2)), stamped(), []config.RedshiftLineageConflict{ - sameFailed, {ExecutionID: "done", Status: config.StatusCompleted, Recommendations: rs}}) + sameFailed, {ExecutionID: "done", Status: config.StatusCompleted, RecentBuy: true, Recommendations: rs}}) require.Error(t, run.err) assert.Equal(t, redshiftRecentBuyRefusal, run.err.Error(), "a recent finished buy wins over the settle text") } @@ -170,7 +170,7 @@ func TestRedshiftDispatchGate_PicksTheRefusalTextByConflictKind(t *testing.T) { rs := []config.RecommendationRecord{gateRec("acct-A", 2)} failed := config.RedshiftLineageConflict{ExecutionID: "f", Status: config.StatusFailed, Recommendations: rs} running := config.RedshiftLineageConflict{ExecutionID: "r", Status: "running", Recommendations: rs} - done := config.RedshiftLineageConflict{ExecutionID: "d", Status: config.StatusCompleted, Recommendations: rs} + done := config.RedshiftLineageConflict{ExecutionID: "d", Status: config.StatusCompleted, RecentBuy: true, Recommendations: rs} for name, tc := range map[string]struct { conflicts []config.RedshiftLineageConflict want string @@ -194,6 +194,27 @@ func TestRedshiftDispatchGate_FailedRowWithRefusedProviderRecDropped(t *testing. require.NoError(t, run.err, "Retry refuses the EC2 rec for good, so the row cannot be retried") } +func TestRedshiftDispatchGate_FailedRecentBuyRowIsKeptAndRefuses(t *testing.T) { + both := []config.RecommendationRecord{{Provider: "aws", Service: "ec2"}, gateRec("acct-A", 2)} + for name, c := range map[string]config.RedshiftLineageConflict{ + "retry-refused mixed row": {ExecutionID: "mixed", Status: config.StatusFailed, Recommendations: both}, + "ops-hint error": {ExecutionID: "stuck", Status: config.StatusFailed, Error: "send failed: SES sandbox active"}, + "retryable failed row": {ExecutionID: "retryable", Status: config.StatusFailed, Recommendations: []config.RecommendationRecord{gateRec("acct-A", 2)}}, + } { + c.RecentBuy = true + run := runGate(t, gateExec(0, gateRec("acct-A", 2)), stamped(), []config.RedshiftLineageConflict{c}) + require.Error(t, run.err, name) + assert.True(t, isBeforeSubmission(run.err), name) + assert.Equal(t, redshiftRecentBuyRefusal, run.err.Error(), name) + + c.RecentBuy = false + if name != "retryable failed row" { + run = runGate(t, gateExec(0, gateRec("acct-A", 2)), stamped(), []config.RedshiftLineageConflict{c}) + require.NoError(t, run.err, "%s: outside the window it is dropped", name) + } + } +} + func TestRedshiftDispatchGate_QueriesOnlySelectedRedshiftTuples(t *testing.T) { unselected := gateRec("acct-A", 7) unselected.Selected = false diff --git a/internal/purchase/redshift_lineage.go b/internal/purchase/redshift_lineage.go index 0273d2fa5..dce8e3693 100644 --- a/internal/purchase/redshift_lineage.go +++ b/internal/purchase/redshift_lineage.go @@ -48,8 +48,10 @@ const ( redshiftNoStampRefusal = "refusing to dispatch: this Redshift purchase has no first dispatch time recorded, so check the account in AWS and start a new purchase" redshiftFailedConflictRefusal = "refusing to dispatch: another purchase of the same Redshift reservation (region, node type, term, payment, count) failed and may still be retried, so retry or settle that purchase, or start this one at least 24h after its first dispatch" redshiftNoJournalRefusal = "refusing to dispatch: this Redshift retry needs the purchase attempt journal, which is not configured, so check the account in AWS and start a new purchase" - redshiftRecentBuyRefusal = "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" - 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" + 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" + 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" ) // redshiftAdoptionWindow mirrors cloud-commitments-go's 5m Redshift dedupe skew @@ -144,13 +146,13 @@ func conflictRefusal(executionID string, conflicts []config.RedshiftLineageConfl return beforeSubmission(errors.New(refusal)) } -// conflictRules is the refusal precedence: another family's recent buy, any open lineage, a -// same-family attempt inside the settle. Failed rows only fall through to pickConflict's last case. +// conflictRules is the refusal precedence: another family's row that may have bought recently, any open lineage, a +// same-family attempt inside the settle. Other failed rows only fall through to pickConflict's last case. var conflictRules = []struct { match func(config.RedshiftLineageConflict) bool refusal string }{ - {func(c config.RedshiftLineageConflict) bool { return !c.SameFamily && redshiftFinishedStatus(c.Status) }, redshiftRecentBuyRefusal}, + {func(c config.RedshiftLineageConflict) bool { return c.RecentBuy }, redshiftRecentBuyRefusal}, {func(c config.RedshiftLineageConflict) bool { return c.Status != config.StatusFailed && (!c.SameFamily || c.Status == "running") }, redshiftConflictRefusal}, @@ -183,11 +185,12 @@ func allFailed(conflicts []config.RedshiftLineageConflict) bool { } // dropPersistentFailures removes failed rows Retry can never re-drive (an operator-fix error or a -// refused provider): they would hold the tuple for the whole horizon with no way out. +// refused provider): they would hold the tuple for the whole horizon with no way out. A row inside +// the adoption window stays: it may have bought, and only waiting clears it. func dropPersistentFailures(conflicts []config.RedshiftLineageConflict) []config.RedshiftLineageConflict { kept := make([]config.RedshiftLineageConflict, 0, len(conflicts)) for _, c := range conflicts { - if !c.SameFamily && c.Status == config.StatusFailed && (ResolveOpsHint(c.Error) != "" || retryRefusedForRecs(c)) { + if !c.SameFamily && !c.RecentBuy && c.Status == config.StatusFailed && (ResolveOpsHint(c.Error) != "" || retryRefusedForRecs(c)) { continue } kept = append(kept, c) @@ -199,15 +202,6 @@ func retryRefusedForRecs(c config.RedshiftLineageConflict) bool { return RedriveRefusalReason(&config.PurchaseExecution{Recommendations: c.Recommendations}) != "" } -// redshiftFinishedStatus reports the statuses whose node may already sit in AWS. -func redshiftFinishedStatus(status string) bool { - switch status { - case config.StatusCompleted, config.StatusPartiallyCompleted, config.StatusRevocationRequested: - return true - } - return false -} - // withDispatchTime defaults RequestedAt to now. A Redshift rec must arrive // with the lineage stamp: defaulting would silently move go's dedupe window. func withDispatchTime(rec config.RecommendationRecord, opts common.PurchaseOptions) (common.PurchaseOptions, error) { diff --git a/known_issues/39_redshift_dedupe_window.md b/known_issues/39_redshift_dedupe_window.md index eac76917f..f078b7ffe 100644 --- a/known_issues/39_redshift_dedupe_window.md +++ b/known_issues/39_redshift_dedupe_window.md @@ -10,7 +10,7 @@ **Why deferred:** The full fix belongs in go (an upper bound or an identity stronger than the tuple), tracked in LeanerCloud/cloud-commitments-go#515. The platform mitigates it for buys it records (below). -**Mitigation (platform-recorded buys):** The Redshift dispatch gate now refuses a purchase when another lineage's completed, partially completed or revocation-requested row 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 completed. +**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. **Remaining:** @@ -19,12 +19,14 @@ - 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. -- A `failed` row without the no-submission proof (reaped mid-buy, or an unknown outcome) may hold a real node but is not refused by this window; if go adopts that node, the accounting stays one node and one completed row. The gate counts such rows as open lineages (below). +- A `failed` row without the no-submission proof (reaped mid-buy, or an unknown outcome) may hold a real node. Inside the window the gate refuses it like a finished row, settled or not and whether or not Retry is refused (#1839); outside it, the gate counts it as an open lineage only while Retry can still re-drive it (below). - Now that the Redshift re-drive guard is lifted, a retry can be refused for up to 24h while another lineage's completed node sits inside its window (liveness only, no double buy). - Plan rows that PR2b refused before submission (failed, `no_submission`, attempt 0) used to hold their ramp step until the account was detached. They are now retryable, so that hold clears with a Retry once the other node leaves its window. **Status:** Mitigated for platform-recorded buys. Tracked in #702 and LeanerCloud/cloud-commitments-go#515. +**History:** #1839 extended the adoption-window refusal to failed rows that may have bought, including settled rows and rows Retry refuses for good. + ## LOW: unknown outcome followed by a new same-tuple purchase outside the window can double-buy **Files:** `internal/purchase/execution.go`, `internal/purchase/manager.go`