From 9b2407fa80a88a9ebc6a9ace06c5fcc28e8bf4d1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 00:15:45 +0100 Subject: [PATCH 1/8] fix(ri-exchange): record unknown for a direct AWS execute only when the outcome is unknown The in-flight guard blocks on unknown rows, so every plain pre-accept error (payment over max, invalid exchange, quote throttle) would have blocked the RI. Record unknown only for errors.Is(err, common.ErrOutcomeUnknown), which classifyAcceptError (pkg/exchange/outcome.go:21-35, go pin 790f369f4822) applies to every accept error except a definite 4xx or throttle. Azure keeps its rule. Refs #1829 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_ri_exchange.go | 9 ++-- internal/api/ri_exchange_audit.go | 25 +++++++++-- internal/api/ri_exchange_audit_test.go | 44 ++++++++++++++++--- ...ri_exchange_direct_outcome_unknown_test.go | 2 +- internal/api/ri_exchange_refusals.go | 8 ++-- .../config/ri_exchange_direct_execution.go | 5 ++- 6 files changed, 75 insertions(+), 18 deletions(-) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 94b3f986b..236125fe4 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -2076,10 +2076,10 @@ func (h *Handler) executeExchange(ctx context.Context, req *events.LambdaFunctio if errors.Is(err, common.ErrOutcomeUnknown) { mapped = directExchangeOutcomeUnknown(body.RIIDs) } - h.finalizeDirectExchange(ctx, auditID, awsDirectExchangeOutcome(exchangeID, quote, err, mapped)) + h.finalizeDirectExchange(ctx, auditID, awsDirectExchangeOutcome(exchangeID, quote, err)) return nil, mapped } - h.finalizeDirectExchange(ctx, auditID, awsDirectExchangeOutcome(exchangeID, quote, nil, nil)) + h.finalizeDirectExchange(ctx, auditID, awsDirectExchangeOutcome(exchangeID, quote, nil)) return &ExchangeExecuteResponse{ ExchangeID: exchangeID, @@ -2120,8 +2120,9 @@ func mapAWSExchangeError(opMsg string, err error) error { // directExchangeOutcomeUnknown is the error for an accept whose response was // lost: AWS may have exchanged the RIs, so the operator must check before -// retrying (#1050). It stays 5xx so the audit records the outcome as unknown, -// and it names only the caller's own RI ids, never provider text. +// retrying (#1050). The audit status is decided from the library's error, +// which must keep its wrapping (awsDirectFailureStatus), not from this status. +// It names only the caller's own RI ids, never provider text. func directExchangeOutcomeUnknown(riIDs []string) error { return NewClientError(502, fmt.Sprintf("the exchange outcome is unknown: AWS may already have exchanged RI(s) %s; check them at AWS before retrying", strings.Join(riIDs, ", "))) } diff --git a/internal/api/ri_exchange_audit.go b/internal/api/ri_exchange_audit.go index 28d475b5a..6379be879 100644 --- a/internal/api/ri_exchange_audit.go +++ b/internal/api/ri_exchange_audit.go @@ -3,11 +3,13 @@ package api import ( "context" "encoding/json" + "errors" "fmt" "math" "math/big" "strconv" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" @@ -72,8 +74,8 @@ func (h *Handler) finalizeDirectExchange(ctx context.Context, id string, outcome // directExchangeFailureStatus classifies a failed provider call for the audit // trail from the error the handler returns for it: a 4xx means the provider // rejected the request as a client fault, so the exchange did not happen; -// anything else (timeout, 5xx, transport error) does not prove that, so the -// exchange may have committed and the outcome is recorded as unknown. +// anything else does not prove that, so the exchange may have committed and +// the outcome is recorded as unknown. Azure only; AWS uses awsDirectFailureStatus. func directExchangeFailureStatus(mapped error) string { if ce, ok := IsClientError(mapped); ok && ce.code >= 400 && ce.code < 500 { return config.RIExchangeDirectStatusFailed @@ -181,7 +183,7 @@ func awsDirectExchangeAudit(session *Session, cloudAccountID string, body Exchan // awsDirectExchangeOutcome builds the outcome of a direct AWS execute. // ExecuteExchange may return its quote alongside an error (a guardrail // refusal), so the quoted amount is recorded whenever there is one. -func awsDirectExchangeOutcome(exchangeID string, quote *exchange.ExchangeQuoteSummary, providerErr, mapped error) config.RIExchangeDirectOutcome { +func awsDirectExchangeOutcome(exchangeID string, quote *exchange.ExchangeQuoteSummary, providerErr error) config.RIExchangeDirectOutcome { outcome := config.RIExchangeDirectOutcome{ Status: config.RIExchangeDirectStatusCompleted, ProviderReference: exchangeID, @@ -191,12 +193,27 @@ func awsDirectExchangeOutcome(exchangeID string, quote *exchange.ExchangeQuoteSu outcome.NetPayableCurrency = quote.CurrencyCode } if providerErr != nil { - outcome.Status = directExchangeFailureStatus(mapped) + outcome.Status = awsDirectFailureStatus(providerErr) outcome.Error = providerErr.Error() } return outcome } +// awsDirectFailureStatus is unknown only for an error the library marked +// common.ErrOutcomeUnknown, because only then may the accept have committed. +// This depends on classifyAcceptError (pkg/exchange/outcome.go:21-35 in the +// pinned cloud-commitments-go/pkg) wrapping every accept error except a +// definite 4xx or throttle, and on pre-accept errors (quote, payment over +// max, invalid exchange, STS) staying unwrapped. The in-flight guard blocks +// on unknown rows, so a plain error must be failed or it would block the RI. +// TestAWSDirectFailureStatus pins that wrapping. +func awsDirectFailureStatus(providerErr error) string { + if errors.Is(providerErr, common.ErrOutcomeUnknown) { + return config.RIExchangeDirectStatusUnknown + } + return config.RIExchangeDirectStatusFailed +} + // azureDirectExchangeAudit builds the audit row for a direct Azure execute // from the request and the fresh re-quote that cleared the guardrails. func azureDirectExchangeAudit(session *Session, accountID string, body AzureExecuteExchangeRequestBody, maxRat *big.Rat, preview *azureExchangePreview) *config.RIExchangeDirectExecution { diff --git a/internal/api/ri_exchange_audit_test.go b/internal/api/ri_exchange_audit_test.go index be6a0973a..a8af0ce18 100644 --- a/internal/api/ri_exchange_audit_test.go +++ b/internal/api/ri_exchange_audit_test.go @@ -12,6 +12,7 @@ import ( "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" azurecompute "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/compute" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" @@ -234,8 +235,9 @@ func TestExecuteExchange_AuditInsertFailureRefusesExchange(t *testing.T) { } // TestExecuteExchange_FailureOutcomeClassification: a provider client fault -// proves the exchange did not happen (failed); any other error does not, so -// the trail must not claim it failed (unknown). +// proves the exchange did not happen (failed), and so does any error from +// before the accept; only an error the library marks ErrOutcomeUnknown may +// have committed (unknown). func TestExecuteExchange_FailureOutcomeClassification(t *testing.T) { tests := []struct { name string @@ -250,17 +252,37 @@ func TestExecuteExchange_FailureOutcomeClassification(t *testing.T) { wantStatus: config.RIExchangeDirectStatusFailed, }, { - name: "transport failure", - err: errors.New("AcceptReservedInstancesExchangeQuote: context deadline exceeded"), + name: "transport failure wrapped as the library does", + err: fmt.Errorf("%w: the accept may have been processed: %w", common.ErrOutcomeUnknown, errors.New("AcceptReservedInstancesExchangeQuote: context deadline exceeded")), wantStatus: config.RIExchangeDirectStatusUnknown, }, + { + name: "unwrapped pre-accept error did not reach the accept", + err: errors.New("GetReservedInstancesExchangeQuote: context deadline exceeded"), + wantStatus: config.RIExchangeDirectStatusFailed, + }, { name: "guardrail refusal keeps the quoted amount", err: errors.New("payment due 900 exceeds max 500"), quote: &exchange.ExchangeQuoteSummary{PaymentDueRaw: "900.000000", CurrencyCode: "USD"}, - wantStatus: config.RIExchangeDirectStatusUnknown, + wantStatus: config.RIExchangeDirectStatusFailed, wantPay: toPtr("900"), }, + { + name: "re-quote over the cap", + err: errors.New("re-quoted payment due 1200 exceeds cap 1000"), + wantStatus: config.RIExchangeDirectStatusFailed, + }, + { + name: "invalid exchange quote", + err: errors.New("exchange is not valid: target offering unavailable"), + wantStatus: config.RIExchangeDirectStatusFailed, + }, + { + name: "throttled accept is a definite reject", + err: &smithy.GenericAPIError{Code: "RequestLimitExceeded", Message: "throttled", Fault: smithy.FaultServer}, + wantStatus: config.RIExchangeDirectStatusFailed, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -533,3 +555,15 @@ func TestAWSDirectExchangeAudit_LegacySingletonTarget(t *testing.T) { assert.Equal(t, "250", audit.MaxPaymentDue) require.NoError(t, audit.Validate()) } + +// TestAWSDirectFailureStatus pins the dependency on the library's +// classifyAcceptError wrapping: a wrapped ErrOutcomeUnknown, however nested, +// is unknown; a plain error is failed. If the library stops wrapping, the +// in-flight guard silently stops protecting, and this is where it shows. +func TestAWSDirectFailureStatus(t *testing.T) { + wrapped := fmt.Errorf("%w: the accept may have been processed: %w", common.ErrOutcomeUnknown, errors.New("send request failed")) + assert.Equal(t, config.RIExchangeDirectStatusUnknown, awsDirectFailureStatus(wrapped)) + assert.Equal(t, config.RIExchangeDirectStatusUnknown, awsDirectFailureStatus(fmt.Errorf("outer: %w", wrapped))) + assert.Equal(t, config.RIExchangeDirectStatusFailed, awsDirectFailureStatus(errors.New("send request failed"))) + assert.Equal(t, config.RIExchangeDirectStatusFailed, awsDirectFailureStatus(&smithy.GenericAPIError{Code: "InternalError", Fault: smithy.FaultServer})) +} diff --git a/internal/api/ri_exchange_direct_outcome_unknown_test.go b/internal/api/ri_exchange_direct_outcome_unknown_test.go index 157443bc1..19f99e29c 100644 --- a/internal/api/ri_exchange_direct_outcome_unknown_test.go +++ b/internal/api/ri_exchange_direct_outcome_unknown_test.go @@ -33,5 +33,5 @@ func TestExecuteExchange_OutcomeUnknownIs502NamingSourceRIs(t *testing.T) { _, outcome, finalized := store.onlyRow(t) require.True(t, finalized) - assert.Equal(t, config.RIExchangeDirectStatusUnknown, outcome.Status, "a 5xx keeps the audit outcome unknown, never failed") + assert.Equal(t, config.RIExchangeDirectStatusUnknown, outcome.Status, "a lost accept response (ErrOutcomeUnknown) keeps the audit outcome unknown, never failed") } diff --git a/internal/api/ri_exchange_refusals.go b/internal/api/ri_exchange_refusals.go index 364c10658..fdc9c82d3 100644 --- a/internal/api/ri_exchange_refusals.go +++ b/internal/api/ri_exchange_refusals.go @@ -33,9 +33,11 @@ var exchangeRefusals = []error{ // exchangeRefusalError returns a 422 carrying err's message when err wraps // one of exchangeRefusals, or nil otherwise. 422 matches the Azure execute // guardrails (checkAzureExchangeMoneyGuardrails), which refuse a currency -// mismatch the same way, and a 4xx makes directExchangeFailureStatus record -// the audit row as failed rather than unknown, which is correct because these -// refusals all happen before the provider's commit call. +// mismatch the same way. On Azure a 4xx makes directExchangeFailureStatus +// record the audit row as failed rather than unknown. +// On AWS the row is failed because the refusal error is not wrapped in +// common.ErrOutcomeUnknown (awsDirectFailureStatus). Both are correct because +// these refusals all happen before the provider's commit call. func exchangeRefusalError(err error) error { for _, sentinel := range exchangeRefusals { if errors.Is(err, sentinel) { diff --git a/internal/config/ri_exchange_direct_execution.go b/internal/config/ri_exchange_direct_execution.go index 985167f34..b2dd0ce8e 100644 --- a/internal/config/ri_exchange_direct_execution.go +++ b/internal/config/ri_exchange_direct_execution.go @@ -12,7 +12,10 @@ import ( // Statuses of an ri_exchange_direct_executions row (issue #82). A row is // written as RIExchangeDirectStatusSubmitted before the provider call and // moved exactly once to one of the three terminal statuses after it; see -// migration 000131 for what each one asserts about the money. +// migration 000131 for what each one asserts about the money. On AWS, +// unknown means only that the accept response was lost (the library wrapped +// the error in common.ErrOutcomeUnknown); migration 000131's "timeout, 5xx, +// transport error" wording is stale for AWS. const ( RIExchangeDirectStatusSubmitted = "submitted" RIExchangeDirectStatusCompleted = "completed" From 394b2b4aebb4a7792abe0315ac1c4654beed0962 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 00:29:44 +0100 Subject: [PATCH 2/8] fix(ri-exchange): refuse reservations of an RI held by an in-flight direct execute Sibling of the processing-row guard: an AWS direct row still submitted or unknown holds its source RIs against ReserveRIExchange and ReserveApprovedRIExchange, under the same advisory lock. It normalises its own input and the stored ids, fails closed on a lookup error, and keeps the blocking row id out of the error. Refs #1829 Co-Authored-By: Claude Opus 5.5 --- internal/config/store_postgres.go | 65 +++++++- .../config/store_postgres_pgxmock_test.go | 3 + ..._exchange_direct_guard_integration_test.go | 156 ++++++++++++++++++ 3 files changed, 222 insertions(+), 2 deletions(-) create mode 100644 internal/config/store_postgres_ri_exchange_direct_guard_integration_test.go diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 52aaddaf6..7fbfe0656 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -3359,7 +3359,7 @@ func (s *PostgresStore) ReserveRIExchange(ctx context.Context, record *RIExchang if reserveErr != nil { return reserveErr } - if guardErr := refuseRIExchangeSourceInFlight(ctx, tx, nil, record.SourceRIIDs); guardErr != nil { + if guardErr := refuseRIExchangeSourcesInFlight(ctx, tx, nil, record.SourceRIIDs); guardErr != nil { return guardErr } candidate.PaymentDue = ceiling @@ -3410,7 +3410,7 @@ func (s *PostgresStore) ReserveApprovedRIExchange(ctx context.Context, id, daily if reserveErr != nil { return reserveErr } - if guardErr := refuseRIExchangeSourceInFlight(ctx, tx, &id, sources); guardErr != nil { + if guardErr := refuseRIExchangeSourcesInFlight(ctx, tx, &id, sources); guardErr != nil { return guardErr } _, updErr := tx.Exec(ctx, @@ -3460,6 +3460,67 @@ func refuseRIExchangeSourceInFlight(ctx context.Context, tx pgx.Tx, excludeID *s return fmt.Errorf("%w: source RI(s) %s already have an exchange in progress; resolve that exchange first", ErrRIExchangeSourceInFlight, strings.Join(shared, ", ")) } +// refuseRIExchangeSourcesInFlight runs both source-RI guards: the processing +// history rows and the in-flight direct executes (#1829). +func refuseRIExchangeSourcesInFlight(ctx context.Context, tx pgx.Tx, excludeID *string, sources []string) error { + if err := refuseRIExchangeSourceInFlight(ctx, tx, excludeID, sources); err != nil { + return err + } + return refuseRIExchangeSourceInFlightDirect(ctx, tx, sources) +} + +// normaliseRIExchangeSourceIDs lowercases and trims ids, dropping empty and +// duplicate ones. +func normaliseRIExchangeSourceIDs(sources []string) []string { + seen := make(map[string]struct{}, len(sources)) + out := make([]string, 0, len(sources)) + for _, src := range sources { + src = strings.ToLower(strings.TrimSpace(src)) + if _, dup := seen[src]; src == "" || dup { + continue + } + seen[src] = struct{}{} + out = append(out, src) + } + return out +} + +// refuseRIExchangeSourceInFlightDirect is the sibling guard for direct +// executes: an AWS ri_exchange_direct_executions row still submitted (the +// accept may be committing) or unknown (it may have committed) holds its +// source RIs. It normalises its own input and compares against the stored ids +// normalised the same way, so spelling and padding cannot slip an RI past it. +// Like refuseRIExchangeSourceInFlight it runs under the daily-spend advisory +// lock, fails closed on a lookup error, and logs the blocking row's id rather +// than returning it (#1428). +func refuseRIExchangeSourceInFlightDirect(ctx context.Context, tx pgx.Tx, sources []string) error { + normalised := normaliseRIExchangeSourceIDs(sources) + if len(normalised) == 0 { + return nil + } + var blocker string + var shared []string + err := tx.QueryRow(ctx, ` + SELECT id::text, ARRAY(SELECT n FROM unnest(ids) n WHERE n = ANY($1::text[]) ORDER BY n) + FROM ( + SELECT id, created_at, ARRAY(SELECT lower(btrim(x, E' \t\n\r\f\x0B')) FROM unnest(source_ids) x) AS ids + FROM ri_exchange_direct_executions + WHERE provider = 'aws' AND status IN ('submitted', 'unknown') + ) d + WHERE ids && $1::text[] + ORDER BY created_at, id + LIMIT 1`, normalised).Scan(&blocker, &shared) + if errors.Is(err, pgx.ErrNoRows) { + return nil + } + if err != nil { + logging.Errorf("RI exchange reservation: direct execute source RI check failed: %v", err) + return errors.New("failed to check RI exchange source RIs held by a direct execute") + } + logging.Warnf("RI exchange reservation refused: source RI(s) %v are held by direct execute %s", shared, blocker) + return fmt.Errorf("%w: source RI(s) %s are held by a direct exchange that may have reached AWS; an operator must verify the outcome at AWS and settle the record", ErrRIExchangeSourceInFlight, strings.Join(shared, ", ")) +} + // ErrRIExchangeDailyCapExceeded marks a reservation refused because the // initial quote does not fit the remaining daily or per-exchange headroom. var ErrRIExchangeDailyCapExceeded = errors.New("RI exchange daily cap exceeded") diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 0f6e76d0f..fa378c975 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -3585,6 +3585,9 @@ func TestPGXMock_ReserveApprovedRIExchange_ReservesCeilingExcludingOwnRow(t *tes // No other processing row shares ri-1, so the source-RI guard (#1050) passes. mock.ExpectQuery("SELECT id::text, ARRAY").WithArgs([]string{"ri-1"}, &id). WillReturnRows(pgxmock.NewRows([]string{"id", "shared"})) + // Nor does an in-flight direct execute (#1829). + mock.ExpectQuery("FROM ri_exchange_direct_executions").WithArgs([]string{"ri-1"}). + WillReturnRows(pgxmock.NewRows([]string{"id", "shared"})) mock.ExpectExec("UPDATE ri_exchange_history SET payment_due").WithArgs(id, "1000.000000"). WillReturnResult(pgxmock.NewResult("UPDATE", 1)) mock.ExpectCommit() diff --git a/internal/config/store_postgres_ri_exchange_direct_guard_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_guard_integration_test.go new file mode 100644 index 000000000..f44844c4f --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_guard_integration_test.go @@ -0,0 +1,156 @@ +//go:build integration + +package config + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Real-PostgreSQL coverage for #1829: an AWS direct execute that may still be +// committing (submitted) or whose outcome is unknown holds its source RIs, so +// no history reservation (AUTO or approval) takes them again. + +// directRow inserts an audit row for the given provider and status, as the +// direct-execute handler would have left it. +func (e *riExchangeIntegrationEnv) directRow(t *testing.T, provider, status string, sources ...string) string { + t.Helper() + var id string + require.NoError(t, e.poolA.QueryRow(e.ctx, ` + INSERT INTO ri_exchange_direct_executions + (id, provider, source_ids, targets, max_payment_due, max_payment_currency, status, executed_by, completed_at) + VALUES (gen_random_uuid(), $1, $2, '[{"offering_id":"o","count":1}]'::jsonb, 100, 'USD', $3, 'tester', + CASE WHEN $3 = 'submitted' THEN NULL ELSE now() END) + RETURNING id::text`, provider, sources, status).Scan(&id)) + return id +} + +func (e *riExchangeIntegrationEnv) reserve(t *testing.T, account string, sources ...string) error { + t.Helper() + cap := e.capAbove(t, time.Now().UTC()) + r := guardCandidate(account, sources...) + _, err := e.storeA.ReserveRIExchange(e.ctx, r, cap, cap) + if err == nil { + e.release(t, r.ID) + } + return err +} + +// [fail-first] A direct row that is submitted or unknown refuses an AUTO +// reservation of its RI, names only the caller's RI, and never the row. +func TestPostgresStoreDB_ReserveRIExchange_RefusesInFlightDirectExecution(t *testing.T) { + for _, status := range []string{"unknown", "submitted"} { + t.Run(status, func(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + blockerID := env.directRow(t, "aws", status, "ri-g1829-a", "ri-g1829-shared") + + account := testAccountID(830000000001) + err := env.reserve(t, account, "ri-g1829-shared", "ri-g1829-c") + require.ErrorIs(t, err, ErrRIExchangeSourceInFlight) + assert.Contains(t, err.Error(), "ri-g1829-shared") + assert.Contains(t, err.Error(), "verify") + assert.NotContains(t, err.Error(), "ri-g1829-a", "only the shared RI is named") + assert.NotContains(t, err.Error(), blockerID, "the blocking row's id is never exposed (#1428)") + assert.Zero(t, env.rowCount(t, account), "a refused reservation writes no row") + }) + } +} + +// [fail-first] An approval of a RI held by a direct row is refused too. +func TestPostgresStoreDB_ReserveApprovedRIExchange_RefusesInFlightDirectExecution(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + cap := env.capAbove(t, time.Now().UTC()) + env.directRow(t, "aws", "unknown", "ri-g1829-approve") + blockerID := env.directRow(t, "aws", "submitted", "ri-g1829-other") + row := env.processingWith(t, testAccountID(830000000011), "ri-g1829-approve") + defer env.release(t, row.ID) + + _, err := env.storeA.ReserveApprovedRIExchange(env.ctx, row.ID, cap, cap) + require.ErrorIs(t, err, ErrRIExchangeSourceInFlight) + assert.Contains(t, err.Error(), "ri-g1829-approve") + assert.NotContains(t, err.Error(), "ri-g1829-other", "only the shared RI is named") + assert.NotContains(t, err.Error(), blockerID, "the blocking row's id is never exposed (#1428)") +} + +// [fail-first] The approval path normalises too: upper case and padded ids +// match a stored lower-case id. +func TestPostgresStoreDB_ReserveApprovedRIExchange_DirectGuardNormalisesIDs(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + cap := env.capAbove(t, time.Now().UTC()) + env.directRow(t, "aws", "unknown", "ri-g1829-abc") + for i, ri := range []string{"RI-G1829-ABC", " RI-G1829-ABC "} { + row := env.processingWith(t, testAccountID(830000000051+int64(i)), ri) + _, err := env.storeA.ReserveApprovedRIExchange(env.ctx, row.ID, cap, cap) + require.ErrorIs(t, err, ErrRIExchangeSourceInFlight, "%q", ri) + env.release(t, row.ID) + } +} + +// [guard] A held RI does not block a different one, on either path. +func TestPostgresStoreDB_DirectGuard_DisjointRIStillReserves(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + cap := env.capAbove(t, time.Now().UTC()) + env.directRow(t, "aws", "unknown", "ri-g1829-x") + + require.NoError(t, env.reserve(t, testAccountID(830000000061), "ri-g1829-y")) + row := env.processingWith(t, testAccountID(830000000062), "ri-g1829-y") + defer env.release(t, row.ID) + _, err := env.storeA.ReserveApprovedRIExchange(env.ctx, row.ID, cap, cap) + require.NoError(t, err) +} + +// [guard] Settled rows and Azure rows do not hold an AWS RI. +func TestPostgresStoreDB_ReserveRIExchange_SettledOrAzureDirectRowsDoNotBlock(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + env.directRow(t, "aws", "completed", "ri-g1829-done") + env.directRow(t, "aws", "failed", "ri-g1829-failed") + env.directRow(t, "azure", "unknown", "ri-g1829-azure") + env.directRow(t, "azure", "submitted", "ri-g1829-azure2") + + for i, ri := range []string{"ri-g1829-done", "ri-g1829-failed", "ri-g1829-azure", "ri-g1829-azure2"} { + require.NoError(t, env.reserve(t, testAccountID(830000000020+int64(i)), ri), ri) + } +} + +// [fail-first] The guard normalises both sides: spelling and surrounding +// spaces cannot slip an RI past it. +func TestPostgresStoreDB_ReserveRIExchange_DirectGuardNormalisesIDs(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + env.directRow(t, "aws", "unknown", "ri-g1829-case") + env.directRow(t, "aws", "unknown", " RI-G1829-SPACE ") + + err := env.reserve(t, testAccountID(830000000031), "RI-G1829-CASE") + require.ErrorIs(t, err, ErrRIExchangeSourceInFlight, "stored lower case, reserved upper case") + err = env.reserve(t, testAccountID(830000000032), "ri-g1829-space") + require.ErrorIs(t, err, ErrRIExchangeSourceInFlight, "stored padded upper case, reserved lower case") + err = env.reserve(t, testAccountID(830000000033), " Ri-G1829-Case ") + require.ErrorIs(t, err, ErrRIExchangeSourceInFlight, "reserved padded mixed case") + + env.directRow(t, "aws", "unknown", "ri-g1829-tab\t") + err = env.reserve(t, testAccountID(830000000034), "ri-g1829-tab") + require.ErrorIs(t, err, ErrRIExchangeSourceInFlight, "stored with a trailing tab") + + // PostgreSQL E-strings have no \v escape: a trim set spelled with it would + // strip the letter v, so an id that ends in v must still match itself. + env.directRow(t, "aws", "unknown", "ri-g1829-endsv") + err = env.reserve(t, testAccountID(830000000035), "ri-g1829-endsv") + require.ErrorIs(t, err, ErrRIExchangeSourceInFlight, "an id ending in v must not be letter-stripped by the trim set") +} + +// [fail-first] A lookup error refuses the reservation instead of letting it +// through. +func TestPostgresStoreDB_ReserveRIExchange_DirectGuardFailsClosedOnLookupError(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + _, err := env.poolA.Exec(env.ctx, "DROP TABLE ri_exchange_direct_executions") + require.NoError(t, err) + + account := testAccountID(830000000041) + err = env.reserve(t, account, "ri-g1829-x") + require.Error(t, err) + assert.NotErrorIs(t, err, ErrRIExchangeSourceInFlight) + assert.Contains(t, err.Error(), "failed to check RI exchange source RIs held by a direct execute", "the guard itself must refuse, not a later statement on the aborted transaction") + assert.Zero(t, env.rowCount(t, account), "a failed lookup writes no row") +} From 09a0eeda9c1e520a4172c6d44a1d1e192fa8e71e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 00:30:01 +0100 Subject: [PATCH 3/8] fix(ri-exchange): let history show the direct-execute guard refusal The sibling guard's refusal is stored whole in ri_exchange_history.error, so the history sanitizer needs a closed pattern for it or it reads back as a generic message. Only the shared RI ids vary. Refs #1829 Co-Authored-By: Claude Opus 5.5 --- internal/purchase/errnotes.go | 6 ++++++ internal/purchase/errnotes_test.go | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index 561045750..44d777e37 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -66,6 +66,9 @@ var bareNotePatterns = []string{ `reserved ceiling "` + shortValuePattern + `" is not a decimal`, `exchange reservation returned no record ID`, `exchange reservation returned invalid ceiling "` + shortValuePattern + `"`, + // config.refuseRIExchangeSourceInFlightDirect refusal, stored whole from + // err.Error() (sentinel text first); only the shared RI ids vary. + `RI exchange source RI is held by an exchange in progress: source RI\(s\) ` + directGuardIDListPattern + ` are held by a direct exchange that may have reached AWS; an operator must verify the outcome at AWS and settle the record`, // Scheduler note for a provider name it does not know. `unknown provider "` + shortValuePattern + `" \(valid: aws, azure, gcp\)`, } @@ -98,6 +101,9 @@ const ( skippedNotePattern = `\d+ recommendation\(s\) without a frozen baseline skipped \(under-buy\): ` + idListPattern ) +// directGuardIDListPattern is the ", "-joined lowercase RI ids the direct-execute guard names. +const directGuardIDListPattern = `[A-Za-z0-9-]{1,64}(?:, [A-Za-z0-9-]{1,64})*` + // serverNoteHeads are server notes whose tail can carry a wrapped store or // config error. Only the fixed head is ever shown; the tail is dropped. var serverNoteHeads = []string{ diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go index 562f9cf2e..d2b430f6f 100644 --- a/internal/purchase/errnotes_test.go +++ b/internal/purchase/errnotes_test.go @@ -261,3 +261,13 @@ func TestSanitizeStoredError_SlotsRejectWhitespaceAndControls(t *testing.T) { assert.Equal(t, in, SanitizeStoredError(in), in) } } + +// The direct-execute source guard's refusal (config.refuseRIExchangeSourceInFlightDirect) +// is stored whole in ri_exchange_history.error and must read back verbatim; any +// extra suffix falls back to the generic message. +func TestSanitizeStoredError_DirectExecuteGuardRefusal(t *testing.T) { + note := "RI exchange source RI is held by an exchange in progress: source RI(s) ri-0abc, ri-0def are held by a direct exchange that may have reached AWS; an operator must verify the outcome at AWS and settle the record" + assert.Equal(t, note, SanitizeStoredError(note)) + assert.NotEqual(t, note+": secret detail", SanitizeStoredError(note+": secret detail")) + assert.NotEqual(t, note+"\nx", SanitizeStoredError(note+"\nx")) +} From dfc9ea3147659040cba75eeb2c165762be892411 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 01:13:15 +0100 Subject: [PATCH 4/8] fix(ri-exchange): guard the direct AWS save under the shared spend lock Save the audit row in one transaction that takes the advisory lock, refuses a source RI held by a processing history row or an in-flight direct row, then inserts with created_at = clock_timestamp() so the lock wait cannot age the row. created_at = clock_timestamp() applies to both providers and is harmless for Azure, which takes no lock and has a short tx. Azure rows are not guarded. Existing shared-database subtests get distinct AWS source ids. Refs #1829 Co-Authored-By: Claude Opus 5.5 --- internal/config/store_postgres.go | 13 +- .../store_postgres_ri_exchange_direct.go | 87 ++++++++--- ...ore_postgres_ri_exchange_direct_db_test.go | 5 + ...stgres_ri_exchange_direct_guard_db_test.go | 137 ++++++++++++++++++ .../store_postgres_ri_exchange_direct_test.go | 27 ++++ 5 files changed, 246 insertions(+), 23 deletions(-) create mode 100644 internal/config/store_postgres_ri_exchange_direct_guard_db_test.go diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 7fbfe0656..4a21201ff 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -3525,13 +3525,22 @@ func refuseRIExchangeSourceInFlightDirect(ctx context.Context, tx pgx.Tx, source // initial quote does not fit the remaining daily or per-exchange headroom. var ErrRIExchangeDailyCapExceeded = errors.New("RI exchange daily cap exceeded") +// lockRIExchangeSpend takes the transaction-scoped advisory lock every RI +// exchange reservation and direct-execute save serialises on. +func lockRIExchangeSpend(ctx context.Context, tx pgx.Tx) error { + if _, err := tx.Exec(ctx, "SELECT pg_advisory_xact_lock($1)", riExchangeDailySpendLockKey); err != nil { + return fmt.Errorf("failed to lock RI exchange daily spend: %w", err) + } + return nil +} + // reserveRIExchangeCeiling takes the global daily-spend lock (held until tx ends) // and returns min(perExchangeCap, dailyCap - spend) as the execution ceiling, // refusing when the initial quote does not fit. excludeID leaves one row's own // amount out of spend so an approval does not count itself twice. func reserveRIExchangeCeiling(ctx context.Context, tx pgx.Tx, excludeID *string, initialText string, initial, dailyCap, perCap *big.Int, dailyCapUSD string) (string, error) { - if _, lockErr := tx.Exec(ctx, "SELECT pg_advisory_xact_lock($1)", riExchangeDailySpendLockKey); lockErr != nil { - return "", fmt.Errorf("failed to lock RI exchange daily spend: %w", lockErr) + if lockErr := lockRIExchangeSpend(ctx, tx); lockErr != nil { + return "", lockErr } var spendText string if scanErr := tx.QueryRow(ctx, riExchangeDailySpendQuery, time.Now().UTC(), excludeID).Scan(&spendText); scanErr != nil { diff --git a/internal/config/store_postgres_ri_exchange_direct.go b/internal/config/store_postgres_ri_exchange_direct.go index 8e0ee3b4b..fc6637167 100644 --- a/internal/config/store_postgres_ri_exchange_direct.go +++ b/internal/config/store_postgres_ri_exchange_direct.go @@ -2,13 +2,22 @@ package config import ( "context" + "errors" "fmt" + "strings" + + "github.com/jackc/pgx/v5" ) // SaveRIExchangeDirectExecution inserts the audit row for a direct-API RI // exchange as "submitted", before the provider call (issue #82). The status -// and created_at are not taken from the record: every row starts submitted, -// stamped by the database clock. +// is not taken from the record: every row starts submitted, and created_at is +// stamped by the database clock after the guards ran. +// +// An AWS row is inserted under the same advisory lock as the history +// reservations, after refusing any source RI another exchange still holds +// (ErrRIExchangeSourceInFlight): a processing history row, or a direct row +// that is submitted or unknown (#1829). Azure rows are not guarded. func (s *PostgresStore) SaveRIExchangeDirectExecution(ctx context.Context, record *RIExchangeDirectExecution) error { if err := record.Validate(); err != nil { return err @@ -18,37 +27,73 @@ func (s *PostgresStore) SaveRIExchangeDirectExecution(ctx context.Context, recor raw := string(record.Sources) sources = &raw } + // clock_timestamp(), not the transaction's now(): the lock wait must not + // age the row, because the runbook treats an old submitted row as stale. query := ` INSERT INTO ri_exchange_direct_executions ( id, provider, cloud_account_id, provider_account_id, region, source_ids, sources, targets, max_payment_due, max_payment_currency, quoted_net_payable, provider_reference, - executed_by_user_id, executed_by, executed_by_api_key_id - ) VALUES ($1, $2, $3, $4, $5, $6, $7::jsonb, $8::jsonb, $9::numeric, $10, $11::numeric, $12, $13, $14, $15) + executed_by_user_id, executed_by, executed_by_api_key_id, created_at + ) VALUES ($1, $2, $3, $4, $5, $6, $7::jsonb, $8::jsonb, $9::numeric, $10, $11::numeric, $12, $13, $14, $15, clock_timestamp()) ` - _, err := s.db.Exec(ctx, query, - record.ID, - record.Provider, - record.CloudAccountID, - record.ProviderAccountID, - record.Region, - record.SourceIDs, - sources, - string(record.Targets), - record.MaxPaymentDue, - record.MaxPaymentCurrency, - record.QuotedNetPayable, - nullStringFromString(record.ProviderReference), - record.ExecutedByUserID, - record.ExecutedBy, - record.ExecutedByAPIKeyID, - ) + err := s.WithTx(ctx, func(tx pgx.Tx) error { + if record.Provider == "aws" { + if err := lockRIExchangeSpend(ctx, tx); err != nil { + return err + } + if err := refuseRIExchangeSourcesInFlight(ctx, tx, nil, riExchangeHistoryGuardIDs(record.SourceIDs)); err != nil { + return err + } + } + _, err := tx.Exec(ctx, query, + record.ID, + record.Provider, + record.CloudAccountID, + record.ProviderAccountID, + record.Region, + record.SourceIDs, + sources, + string(record.Targets), + record.MaxPaymentDue, + record.MaxPaymentCurrency, + record.QuotedNetPayable, + nullStringFromString(record.ProviderReference), + record.ExecutedByUserID, + record.ExecutedBy, + record.ExecutedByAPIKeyID, + ) + return err + }) + if errors.Is(err, ErrRIExchangeSourceInFlight) { + return err + } if err != nil { return fmt.Errorf("failed to save ri exchange direct execution: %w", err) } return nil } +// riExchangeHistoryGuardIDs is the input the direct Save gives the history +// guard, which compares exactly: the trimmed raw ids plus their lower-cased +// forms, de-duplicated in order, so a history row stored in either spelling +// of a lower-case id still matches. +func riExchangeHistoryGuardIDs(sourceIDs []string) []string { + seen := make(map[string]struct{}, 2*len(sourceIDs)) + out := make([]string, 0, 2*len(sourceIDs)) + for _, id := range sourceIDs { + id = strings.TrimSpace(id) + for _, v := range []string{id, strings.ToLower(id)} { + if _, dup := seen[v]; v == "" || dup { + continue + } + seen[v] = struct{}{} + out = append(out, v) + } + } + return out +} + // FinalizeRIExchangeDirectExecution records the provider outcome on a // submitted audit row. It is a compare-and-set on status = 'submitted', so a // row is finalized at most once: a second finalize, concurrent or later, diff --git a/internal/config/store_postgres_ri_exchange_direct_db_test.go b/internal/config/store_postgres_ri_exchange_direct_db_test.go index 19afe3d9c..4b09f6c5f 100644 --- a/internal/config/store_postgres_ri_exchange_direct_db_test.go +++ b/internal/config/store_postgres_ri_exchange_direct_db_test.go @@ -49,6 +49,11 @@ func newDirectExec(provider string) *RIExchangeDirectExecution { MaxPaymentCurrency: "USD", ExecutedBy: "alice@example.com", } + if provider == "aws" { + // The in-flight guard (#1829) refuses a second AWS row on a held RI, and + // these subtests share one database, so each gets its own source RI. + rec.SourceIDs = []string{"src-" + rec.ID} + } if provider == "azure" { rec.ProviderAccountID = strPtr("sub-1") rec.Sources = json.RawMessage(`[{"reservation_id":"src-1","quantity":4}]`) diff --git a/internal/config/store_postgres_ri_exchange_direct_guard_db_test.go b/internal/config/store_postgres_ri_exchange_direct_guard_db_test.go new file mode 100644 index 000000000..6ffd06f28 --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_guard_db_test.go @@ -0,0 +1,137 @@ +//go:build integration + +package config + +import ( + "encoding/json" + "fmt" + "sync" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Real-PostgreSQL coverage for #1829: the direct Save runs under the shared +// advisory lock and refuses an AWS source RI that another exchange holds. + +func awsDirectSave(sources ...string) *RIExchangeDirectExecution { + rec := newDirectExec("aws") + rec.SourceIDs = sources + return rec +} + +func (e *riExchangeIntegrationEnv) directRows(t *testing.T) int { + t.Helper() + var n int + require.NoError(t, e.poolA.QueryRow(e.ctx, "SELECT count(*) FROM ri_exchange_direct_executions").Scan(&n)) + return n +} + +func TestRIExchangeDirectSave_GuardRefusesHeldRIs(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + history := env.processingWith(t, testAccountID(840000000001), "ri-g1829-hist") + defer env.release(t, history.ID) + + err := env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave("ri-g1829-hist", "ri-g1829-free")) + require.ErrorIs(t, err, ErrRIExchangeSourceInFlight) + assert.Zero(t, env.directRows(t), "a refused save inserts no audit row") + + held := awsDirectSave("ri-g1829-held") + require.NoError(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, held), "a submitted row") + require.ErrorIs(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave("ri-g1829-held")), ErrRIExchangeSourceInFlight) + require.NoError(t, env.storeA.FinalizeRIExchangeDirectExecution(env.ctx, held.ID, RIExchangeDirectOutcome{Status: RIExchangeDirectStatusUnknown, Error: "lost"})) + require.ErrorIs(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave("ri-g1829-held")), ErrRIExchangeSourceInFlight, "an unknown row") +} + +func TestRIExchangeDirectSave_GuardLeavesOthersAlone(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + history := env.processingWith(t, testAccountID(840000000011), "ri-g1829-hist") + defer env.release(t, history.ID) + env.directRow(t, "aws", "unknown", "ri-g1829-aws") + + azure := newDirectExec("azure") + azure.SourceIDs = []string{"ri-g1829-hist", "ri-g1829-aws"} + require.NoError(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, azure), "Azure is not guarded") + require.NoError(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave("ri-g1829-disjoint"))) + + env.directRow(t, "aws", "completed", "ri-g1829-done") + require.NoError(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave("ri-g1829-done")), "a settled row holds nothing") +} + +func TestRIExchangeDirectSave_GuardNormalisesIDs(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + history := env.processingWith(t, testAccountID(840000000021), "ri-g1829-abc") + defer env.release(t, history.ID) + for _, ri := range []string{"RI-G1829-ABC", " RI-G1829-ABC "} { + require.ErrorIs(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave(ri)), ErrRIExchangeSourceInFlight, "history row, save %q", ri) + } + + env.directRow(t, "aws", "unknown", " RI-G1829-PAD ") + env.directRow(t, "aws", "unknown", "ri-g1829-low") + require.ErrorIs(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave("ri-g1829-pad")), ErrRIExchangeSourceInFlight, "stored padded upper case") + require.ErrorIs(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave("RI-G1829-LOW")), ErrRIExchangeSourceInFlight, "stored lower case") +} + +func TestRIExchangeDirectSave_LockSerialisesAndRunsAtReadCommitted(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + holder, err := env.poolA.Begin(env.ctx) + require.NoError(t, err) + var iso string + require.NoError(t, holder.QueryRow(env.ctx, "SHOW transaction_isolation").Scan(&iso)) + assert.Equal(t, "read committed", iso, "the lock design assumes each statement sees the latest commits") + _, err = holder.Exec(env.ctx, "SELECT pg_advisory_xact_lock($1)", riExchangeDailySpendLockKey) + require.NoError(t, err) + pid := testhelpers.BackendPID(env.ctx, t, holder) + + rec := awsDirectSave("ri-g1829-lock") + done := make(chan error, 1) + go func() { done <- env.storeB.SaveRIExchangeDirectExecution(env.ctx, rec) }() + if got, answered := testhelpers.AwaitBlockedBy(env.ctx, t, env.poolB, pid, done); answered { + _ = holder.Rollback(env.ctx) + t.Fatalf("the save answered (%v) while the lock was held; it must wait for it", got) + } + var releasedAt time.Time + require.NoError(t, holder.QueryRow(env.ctx, "SELECT clock_timestamp()").Scan(&releasedAt)) + require.NoError(t, holder.Commit(env.ctx)) + require.NoError(t, <-done) + + var createdAt time.Time + require.NoError(t, env.poolA.QueryRow(env.ctx, "SELECT created_at FROM ri_exchange_direct_executions WHERE id = $1", rec.ID).Scan(&createdAt)) + assert.False(t, createdAt.Before(releasedAt), "created_at %s predates the lock release %s: the lock wait aged the row", createdAt, releasedAt) +} + +func TestRIExchangeDirectSave_RaceExactlyOneWins(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + const parties = 6 + var wg sync.WaitGroup + errs := make([]error, parties) + start := make(chan struct{}) + for i := 0; i < parties; i++ { + rec := awsDirectSave("ri-g1829-race", fmt.Sprintf("ri-g1829-own-%d", i)) + rec.Targets = json.RawMessage(fmt.Sprintf(`[{"offering_id":"o%d","count":%d}]`, i, i+1)) + rec.ID = uuid.NewString() + wg.Add(1) + go func(i int) { + defer wg.Done() + <-start + errs[i] = env.storeA.SaveRIExchangeDirectExecution(env.ctx, rec) + }(i) + } + close(start) + wg.Wait() + + wins := 0 + for _, err := range errs { + if err == nil { + wins++ + continue + } + assert.ErrorIs(t, err, ErrRIExchangeSourceInFlight) + } + assert.Equal(t, 1, wins, "exactly one save may take the RI") + assert.Equal(t, 1, env.directRows(t)) +} diff --git a/internal/config/store_postgres_ri_exchange_direct_test.go b/internal/config/store_postgres_ri_exchange_direct_test.go index 1bf704c98..a0513f2a7 100644 --- a/internal/config/store_postgres_ri_exchange_direct_test.go +++ b/internal/config/store_postgres_ri_exchange_direct_test.go @@ -41,6 +41,7 @@ func TestPGXMock_SaveRIExchangeDirectExecution_InsertsSubmittedRow(t *testing.T) store := storeWith(mock) rec := validDirectExecution() + mock.ExpectBegin() mock.ExpectExec("INSERT INTO ri_exchange_direct_executions"). WithArgs( directExecTestID, "azure", rec.CloudAccountID, rec.ProviderAccountID, rec.Region, @@ -50,6 +51,30 @@ func TestPGXMock_SaveRIExchangeDirectExecution_InsertsSubmittedRow(t *testing.T) rec.ExecutedByUserID, "alice@example.com", rec.ExecutedByAPIKeyID, ). WillReturnResult(pgxmock.NewResult("INSERT", 1)) + mock.ExpectCommit() + + require.NoError(t, store.SaveRIExchangeDirectExecution(context.Background(), rec)) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// An AWS save takes the shared lock, runs both source-RI guards, then inserts. +func TestPGXMock_SaveRIExchangeDirectExecution_AWSLocksAndGuardsBeforeInsert(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + rec := validDirectExecution() + rec.Provider = "aws" + rec.SourceIDs = []string{" RI-1 "} + + mock.ExpectBegin() + mock.ExpectExec("SELECT pg_advisory_xact_lock").WithArgs(riExchangeDailySpendLockKey). + WillReturnResult(pgxmock.NewResult("SELECT", 1)) + mock.ExpectQuery("SELECT id::text, ARRAY").WithArgs([]string{"RI-1", "ri-1"}, (*string)(nil)). + WillReturnRows(pgxmock.NewRows([]string{"id", "shared"})) + mock.ExpectQuery("FROM ri_exchange_direct_executions").WithArgs([]string{"ri-1"}). + WillReturnRows(pgxmock.NewRows([]string{"id", "shared"})) + mock.ExpectExec("INSERT INTO ri_exchange_direct_executions").WithArgs(anyArgsCfg(15)...). + WillReturnResult(pgxmock.NewResult("INSERT", 1)) + mock.ExpectCommit() require.NoError(t, store.SaveRIExchangeDirectExecution(context.Background(), rec)) assert.NoError(t, mock.ExpectationsWereMet()) @@ -86,9 +111,11 @@ func TestPGXMock_SaveRIExchangeDirectExecution_RejectsInvalidRecord(t *testing.T func TestPGXMock_SaveRIExchangeDirectExecution_DBErrorSurfaces(t *testing.T) { mock := newMock(t) store := storeWith(mock) + mock.ExpectBegin() mock.ExpectExec("INSERT INTO ri_exchange_direct_executions"). WithArgs(anyArgsCfg(15)...). WillReturnError(errors.New("connection refused")) + mock.ExpectRollback() err := store.SaveRIExchangeDirectExecution(context.Background(), validDirectExecution()) require.Error(t, err) From b3dba0e8bb3e34851104c8b4b11b4422514f4aec Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 01:19:27 +0100 Subject: [PATCH 5/8] fix(ri-exchange): refuse a direct execute of a held RI with a 409 before the claim The guarded audit save already runs before the submit claim and the AWS call. Map its in-flight refusal to a 409 that names only the caller's RIs, so nothing is claimed or sent to AWS. Adds the issue repro and the reverse (lost accept response holds the RI against AUTO) against a real database. Refs #1829 Co-Authored-By: Claude Opus 5.5 --- ..._exchange_direct_guard_integration_test.go | 98 +++++++++++++++++++ .../handler_ri_exchange_direct_guard_test.go | 50 ++++++++++ internal/api/openapi.yaml | 14 ++- internal/api/ri_exchange_audit.go | 4 + 4 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 internal/api/handler_ri_exchange_direct_guard_integration_test.go create mode 100644 internal/api/handler_ri_exchange_direct_guard_test.go diff --git a/internal/api/handler_ri_exchange_direct_guard_integration_test.go b/internal/api/handler_ri_exchange_direct_guard_integration_test.go new file mode 100644 index 000000000..b8942b9b2 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_guard_integration_test.go @@ -0,0 +1,98 @@ +//go:build integration + +package api + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const guardAcct = "210987654321" + +// #1829 against a real database: the issue's repro and its reverse. + +func directGuardHandler(t *testing.T, store *config.PostgresStore, executor awsExecutor) (*Handler, *int) { + t.Helper() + calls := new(int) + return &Handler{ + auth: auditSessionAuth(t), + config: store, + reshapeAccountResolver: func(context.Context) (string, error) { return auditCloudAccount, nil }, + executeExchangeFn: func(ctx context.Context, req exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error) { + *calls++ + return executor(ctx, req) + }, + }, calls +} + +func countRows(ctx context.Context, t *testing.T, c *testhelpers.PostgresContainer, table string) int { + t.Helper() + var n int + require.NoError(t, c.DB.QueryRow(ctx, "SELECT count(*) FROM "+table).Scan(&n)) + return n +} + +// Repro: a processing history row holds ri-0a1b2c3d, so the direct execute is +// refused with a 409 and leaves no audit row, no claim and no AWS call. +func TestExecuteExchange_DirectGuard_Repro(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequireMigratedDatabase(ctx, t, getMigrationsPath()) + store := config.NewPostgresStore(container.DB) + require.NoError(t, store.SaveRIExchangeRecord(ctx, &config.RIExchangeRecord{ + AccountID: guardAcct, Region: "eu-central-1", SourceRIIDs: []string{"ri-0a1b2c3d"}, SourceInstanceType: "m5.large", + SourceCount: 1, TargetOfferingID: "o", TargetInstanceType: "m6i.large", TargetCount: 1, PaymentDue: "10.00", Status: "processing", Mode: "auto", + })) + h, calls := directGuardHandler(t, store, func(context.Context, exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error) { + return "riex-1", nil, nil + }) + + _, err := h.executeExchange(auditRequest(ctx, awsAuditBody)) + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.message, "ri-0a1b2c3d") + assert.Zero(t, *calls, "AWS must not be called") + assert.Zero(t, countRows(ctx, t, container, "ri_exchange_direct_executions")) + assert.Zero(t, countRows(ctx, t, container, "ri_exchange_idempotency")) +} + +// Reverse: a direct execute whose accept response is lost leaves an unknown +// row, which then holds its RIs against AUTO and approval reservations. +func TestExecuteExchange_DirectGuard_Reverse(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequireMigratedDatabase(ctx, t, getMigrationsPath()) + store := config.NewPostgresStore(container.DB) + h, calls := directGuardHandler(t, store, func(context.Context, exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error) { + return "", nil, common.ErrOutcomeUnknown + }) + + _, err := h.executeExchange(auditRequest(ctx, awsAuditBody)) + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, 502, ce.code) + assert.Equal(t, 1, *calls) + var status string + require.NoError(t, container.DB.QueryRow(ctx, "SELECT status FROM ri_exchange_direct_executions").Scan(&status)) + assert.Equal(t, config.RIExchangeDirectStatusUnknown, status) + + record := func(ri string) *config.RIExchangeRecord { + return &config.RIExchangeRecord{AccountID: guardAcct, Region: "eu-central-1", SourceRIIDs: []string{ri}, SourceInstanceType: "m5.large", + SourceCount: 1, TargetOfferingID: "o", TargetInstanceType: "m6i.large", TargetCount: 1, PaymentDue: "10.00", Status: "processing", Mode: "auto"} + } + _, err = store.ReserveRIExchange(ctx, record("ri-0a1b2c3d"), "1000", "1000") + require.ErrorIs(t, err, config.ErrRIExchangeSourceInFlight, "AUTO reservation of the held RI") + _, err = store.ReserveRIExchange(ctx, record("ri-other"), "1000", "1000") + require.NoError(t, err, "a different RI still reserves") + + approving := record("ri-0a1b2c3d") + require.NoError(t, store.SaveRIExchangeRecord(ctx, approving)) + _, err = store.ReserveApprovedRIExchange(ctx, approving.ID, "5000", "1000") + require.ErrorIs(t, err, config.ErrRIExchangeSourceInFlight, "approval of the held RI") +} diff --git a/internal/api/handler_ri_exchange_direct_guard_test.go b/internal/api/handler_ri_exchange_direct_guard_test.go new file mode 100644 index 000000000..5b4355dbb --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_guard_test.go @@ -0,0 +1,50 @@ +package api + +import ( + "context" + "fmt" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// #1829: a direct execute whose source RI is held by another exchange is +// refused with a 409 before the submit claim and before any AWS call. +func TestExecuteExchange_DirectGuard_RefusesWith409BeforeClaimAndAWS(t *testing.T) { + store := newDirectExchangeAuditStore() + store.saveErr = fmt.Errorf("%w: source RI(s) ri-0a1b2c3d are held by a direct exchange that may have reached AWS; an operator must verify the outcome at AWS and settle the record", config.ErrRIExchangeSourceInFlight) + h := awsAuditHandler(t, store, func(context.Context, exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error) { + t.Fatal("AWS must not be called for a refused direct execute") + return "", nil, nil + }) + + _, err := h.executeExchange(auditRequest(context.Background(), awsAuditBody)) + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.message, "ri-0a1b2c3d") + // The handler passes the store's text through, so the no-blocker-id property + // belongs to the store; it is asserted there (config guard integration tests). + assert.Equal(t, []string{"save"}, store.recordedSteps(), "no claim, no execute, no finalize") + assert.Empty(t, store.claimedKeys()) +} + +// Any other save failure still refuses the exchange, but as a server error. +func TestExecuteExchange_DirectGuard_OtherSaveErrorsStay5xx(t *testing.T) { + store := newDirectExchangeAuditStore() + store.saveErr = fmt.Errorf("connection refused") + h := awsAuditHandler(t, store, func(context.Context, exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error) { + t.Fatal("AWS must not be called when the audit row cannot be written") + return "", nil, nil + }) + + _, err := h.executeExchange(auditRequest(context.Background(), awsAuditBody)) + require.Error(t, err) + if ce, ok := IsClientError(err); ok { + assert.GreaterOrEqual(t, ce.code, 500) + } + assert.Equal(t, []string{"save"}, store.recordedSteps()) +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 602a538b6..e3d7aa68b 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2545,6 +2545,11 @@ paths: not part of that fingerprint: raising the cap does not make it a different purchase. See the 409 response: it does not assert that the earlier submit succeeded, only that it claimed the window. + The 409 also means a source RI is held by an exchange in progress: an + in-flight approve or AUTO exchange row, or an unresolved direct + execution (submitted, or unknown after a lost accept response). Nothing + is sent to AWS or claimed in that case; verify the outcome at AWS and + have an operator settle the held record before retrying. The exchange runs in the deployment's own AWS account, so a session whose allowed accounts do not cover that account gets 403. A quote that is not denominated in USD, or names no currency, is @@ -2624,7 +2629,14 @@ paths: schema: $ref: '#/components/schemas/Error' '409': - $ref: '#/components/responses/Conflict' + description: >- + Either an identical submit is already inside the 15 minute dedup + window, or a source RI is held by an exchange in progress (an + in-flight approve or AUTO row, or an unresolved direct execution). + content: + application/json: + schema: + $ref: '#/components/schemas/Error' '422': $ref: '#/components/responses/UnprocessableEntity' diff --git a/internal/api/ri_exchange_audit.go b/internal/api/ri_exchange_audit.go index 6379be879..763796862 100644 --- a/internal/api/ri_exchange_audit.go +++ b/internal/api/ri_exchange_audit.go @@ -46,6 +46,10 @@ func (h *Handler) beginDirectExchange(ctx context.Context, audit *config.RIExcha return "", fmt.Errorf("invalid RI exchange audit record, exchange not executed: %w", err) } if err := h.config.SaveRIExchangeDirectExecution(ctx, audit); err != nil { + if errors.Is(err, config.ErrRIExchangeSourceInFlight) { + logging.Warnf("ri-exchange direct execute refused: %v", err) + return "", NewClientError(409, err.Error()) + } logging.Errorf("ri-exchange direct execute: failed to record audit row, refusing to execute: %v", err) return "", fmt.Errorf("failed to record the RI exchange audit trail, exchange not executed: %w", err) } From 2933fdce7366185d5eb8a37356ee18a72790310a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 02:17:38 +0100 Subject: [PATCH 6/8] docs(ri-exchange): runbook and known issues for stuck direct exchange rows Add the DBA settle statement for a direct row left submitted or unknown, with a test that runs the runbook's own SQL against fixtures, and record the cap, resolve-endpoint and id-case limitations. Refs #1829 Co-Authored-By: Claude Opus 5.5 --- ...xchange_direct_runbook_integration_test.go | 62 +++++++++++++++++ known-issues.md | 22 +++++++ .../ri-exchange-direct-execution-settle.md | 66 +++++++++++++++++++ 3 files changed, 150 insertions(+) create mode 100644 internal/config/store_postgres_ri_exchange_direct_runbook_integration_test.go create mode 100644 runbooks/ri-exchange-direct-execution-settle.md diff --git a/internal/config/store_postgres_ri_exchange_direct_runbook_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_runbook_integration_test.go new file mode 100644 index 000000000..6dc3d74bd --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_runbook_integration_test.go @@ -0,0 +1,62 @@ +//go:build integration + +package config + +import ( + "os" + "regexp" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// runbookSettleSQL reads the settle statement from the runbook itself, so the +// operator's copy and the tested statement cannot drift. The :name placeholders +// become positional parameters. +func runbookSettleSQL(t *testing.T) string { + t.Helper() + raw, err := os.ReadFile("../../runbooks/ri-exchange-direct-execution-settle.md") + require.NoError(t, err) + m := regexp.MustCompile("(?s)```sql\n(-- :id[^`]*)```").FindSubmatch(raw) + require.NotNil(t, m, "runbook has no sql block") + return strings.NewReplacer(":new_status", "$1::text", ":id", "$2::uuid", ":note", "$3::text").Replace(string(m[1])) +} + +// #1829 runbook: the settle statement releases only rows it is safe to release. +func TestRIExchangeDirectRunbook_SettleStatement(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + sql := runbookSettleSQL(t) + age := func(id, interval string) { + _, err := env.poolA.Exec(env.ctx, "UPDATE ri_exchange_direct_executions SET created_at = now() - $2::interval WHERE id = $1", id, interval) + require.NoError(t, err) + } + settle := func(id string) int64 { + tag, err := env.poolA.Exec(env.ctx, sql, "failed", id, "checked at AWS") + require.NoError(t, err) + return tag.RowsAffected() + } + + unknown := env.directRow(t, "aws", "unknown", "ri-g1829-rb-unknown") + age(unknown, "1 minute") + assert.EqualValues(t, 1, settle(unknown), "a fresh unknown row is settled") + assert.EqualValues(t, 0, settle(unknown), "a settled row is not settled twice") + var status, errText string + require.NoError(t, env.poolA.QueryRow(env.ctx, "SELECT status, error FROM ri_exchange_direct_executions WHERE id = $1", unknown).Scan(&status, &errText)) + assert.Equal(t, "failed", status) + assert.Contains(t, errText, "checked at AWS") + require.NoError(t, env.reserve(t, testAccountID(850000000001), "ri-g1829-rb-unknown"), "a settled row stops blocking the guard") + + old := env.directRow(t, "aws", "submitted", "ri-g1829-rb-old") + age(old, "6 minutes") + assert.EqualValues(t, 1, settle(old), "a submitted row older than 5 minutes is settled") + + young := env.directRow(t, "aws", "submitted", "ri-g1829-rb-young") + age(young, "1 minute") + assert.EqualValues(t, 0, settle(young), "a submitted row that may still be in flight is not settled") + + assert.EqualValues(t, 0, settle(env.directRow(t, "azure", "unknown", "ri-g1829-rb-azure")), "Azure rows are untouched") + assert.EqualValues(t, 0, settle(env.directRow(t, "aws", "completed", "ri-g1829-rb-done")), "a completed row is final") + assert.EqualValues(t, 0, settle(env.directRow(t, "aws", "failed", "ri-g1829-rb-failed")), "a failed row is final") +} diff --git a/known-issues.md b/known-issues.md index c17ed2597..dd115a3b1 100644 --- a/known-issues.md +++ b/known-issues.md @@ -137,6 +137,28 @@ as an explicitly consented step (the `az role assignment create` mirror of step 1) and must not be reintroduced into the template. `scripts/check-azure-role-parity.sh` fails CI if it is. +### Direct RI exchange spend is outside the daily cap (issue #1829) + +By design (migration 000131's header), a direct execute is bounded only by its +own `max_payment_due`; it is not counted in the daily or per-exchange cap that +AUTO and approved exchanges share, so repeated direct executes can spend past it. +The in-flight source-RI guard only stops two exchanges taking the same RI; it does +not cap total spend. + +### No operator resolve endpoint for direct RI exchange rows (issue #1829 follow-up) + +A direct row left `submitted` or `unknown` keeps its source RIs blocked until a DBA +settles it with the SQL in `runbooks/ri-exchange-direct-execution-settle.md`. A +resolve endpoint (and a list of unresolved direct rows) is tracked in #1836. + +### RI id matching is exact for stored history rows (issue #1829) + +The direct-execute guard normalises (lower, trim) its input and the stored direct +ids, but the history guard compares `source_ri_ids` exactly. A history row whose +stored id is not lowercase matches only identically spelled input. History ids +come from AWS describe calls, which return lowercase RI ids, so this does not occur +in practice; closing it means normalising both sides inside the history guard. + ## Resolved ### Azure diff --git a/runbooks/ri-exchange-direct-execution-settle.md b/runbooks/ri-exchange-direct-execution-settle.md new file mode 100644 index 000000000..96706deef --- /dev/null +++ b/runbooks/ri-exchange-direct-execution-settle.md @@ -0,0 +1,66 @@ +# Runbook: settle a stuck direct RI exchange audit row + +A direct RI exchange (`POST` execute) writes a row in `ri_exchange_direct_executions` +before it calls AWS. While the row is `submitted` or `unknown`, its source RIs are +held: no further direct execute, AUTO exchange or approval may take them (issue 1829), +because the exchange may already have happened at AWS. + +- `submitted`: the process died, or the terminal write failed, before the outcome was recorded. +- `unknown`: the accept call failed without proving a rejection (the library's + `ErrOutcomeUnknown`: a 5xx, timeout or transport error after the request may have + reached AWS). On AWS, `unknown` means only that; a plain pre-accept error is + recorded `failed`. Migration 000131's wording on `unknown` is stale for AWS. + +The refusal the operator sees names the shared RIs and says to verify the outcome at AWS. + +## Step 1: confirm the outcome at AWS + +Run `DescribeReservedInstances` (and check the target offering) for the source RIs +named in the refusal. Decide: + +- no exchange happened: settle as `failed`; +- the exchange happened: settle as `completed`. + +Do not guess. Marking a row `failed` releases the RIs for another exchange. + +## Step 2: find the audit row id + +The refusal hides the blocking row's id. Find it in the server log (search for +"held by direct execute"), or query by the RI id named in the refusal (`:ri`): + +```sql +SELECT id, status, created_at + FROM ri_exchange_direct_executions + WHERE provider = 'aws' AND status IN ('submitted', 'unknown') + AND EXISTS (SELECT 1 FROM unnest(source_ids) x + WHERE lower(btrim(x, E' \t\n\r\f\x0B')) = lower(btrim(:ri, E' \t\n\r\f\x0B'))); +``` + +## Step 3: settle the row (the interim DBA step) + +This rewrites rows that the audit design otherwise treats as final. It is the +interim until the resolve path (#1836) exists. Run only after Step 1. + +```sql +-- :id = the audit row id; :new_status = 'failed' (AWS shows no exchange) or 'completed' (AWS shows it happened) +UPDATE ri_exchange_direct_executions + SET status = :new_status, + completed_at = COALESCE(completed_at, now()), + error = concat_ws(E'\n', error, 'operator-settled ' || now()::text || ': ' || :note) + WHERE id = :id + AND provider = 'aws' + AND status IN ('submitted', 'unknown') + AND (status = 'unknown' OR created_at < now() - interval '5 minutes'); +-- Expect exactly 1 row updated. 0 rows means: wrong id, already settled, or a 'submitted' row younger +-- than 5 minutes that may still be in flight. Do NOT widen the WHERE clause; wait and re-check. +``` + +- **Why the age check:** the AWS call can run for 4 minutes (`riExchangeExecutionTimeout`), + so a `submitted` row younger than 5 minutes (4 minutes plus 1 minute of margin) may + still be in flight. Marking it `failed` would release the RI while AWS might still + commit the accept, and the handler's own finalize would then lose the real outcome. +- **Why `created_at` can be trusted:** it is taken with `clock_timestamp()` after the + guard lock is acquired, so a lock wait does not age the row. A `submitted` row older + than 5 minutes is past the execution window plus the claim. +- **Why `error` is appended:** it keeps the original failure text. +- An `unknown` row is settled at any age: its accept already returned. From f80af7aae10292548397d2ee4e96c181c3e07d2b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 03:38:33 +0100 Subject: [PATCH 7/8] style(config): spell serializes the US way in the spend-lock comment Lint Code (misspell) flagged serialises. Refs #1829 Co-Authored-By: Claude Opus 5.5 --- internal/config/store_postgres.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 4a21201ff..6e13cb0d4 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -3526,7 +3526,7 @@ func refuseRIExchangeSourceInFlightDirect(ctx context.Context, tx pgx.Tx, source var ErrRIExchangeDailyCapExceeded = errors.New("RI exchange daily cap exceeded") // lockRIExchangeSpend takes the transaction-scoped advisory lock every RI -// exchange reservation and direct-execute save serialises on. +// exchange reservation and direct-execute save serializes on. func lockRIExchangeSpend(ctx context.Context, tx pgx.Tx) error { if _, err := tx.Exec(ctx, "SELECT pg_advisory_xact_lock($1)", riExchangeDailySpendLockKey); err != nil { return fmt.Errorf("failed to lock RI exchange daily spend: %w", err) From 57f0711ecb57cb16ab30f0d9e988cb6c0d801703 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 03:47:57 +0100 Subject: [PATCH 8/8] docs(ri-exchange): quote the runbook psql placeholders psql substitutes :x raw, so a uuid or string value needs :'x'. The runbook now uses the quoted form and shows how to set the variables; the runbook test maps the quoted placeholders. Refs #1829 Co-Authored-By: Claude Opus 5.5 --- ...s_ri_exchange_direct_runbook_integration_test.go | 8 ++++---- runbooks/ri-exchange-direct-execution-settle.md | 13 +++++++------ 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/internal/config/store_postgres_ri_exchange_direct_runbook_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_runbook_integration_test.go index 6dc3d74bd..90831620d 100644 --- a/internal/config/store_postgres_ri_exchange_direct_runbook_integration_test.go +++ b/internal/config/store_postgres_ri_exchange_direct_runbook_integration_test.go @@ -13,15 +13,15 @@ import ( ) // runbookSettleSQL reads the settle statement from the runbook itself, so the -// operator's copy and the tested statement cannot drift. The :name placeholders -// become positional parameters. +// operator's copy and the tested statement cannot drift. The :'name' psql +// placeholders become positional parameters. func runbookSettleSQL(t *testing.T) string { t.Helper() raw, err := os.ReadFile("../../runbooks/ri-exchange-direct-execution-settle.md") require.NoError(t, err) - m := regexp.MustCompile("(?s)```sql\n(-- :id[^`]*)```").FindSubmatch(raw) + m := regexp.MustCompile("(?s)```sql\n(-- :'id'[^`]*)```").FindSubmatch(raw) require.NotNil(t, m, "runbook has no sql block") - return strings.NewReplacer(":new_status", "$1::text", ":id", "$2::uuid", ":note", "$3::text").Replace(string(m[1])) + return strings.NewReplacer(":'new_status'", "$1::text", ":'id'", "$2::uuid", ":'note'", "$3::text").Replace(string(m[1])) } // #1829 runbook: the settle statement releases only rows it is safe to release. diff --git a/runbooks/ri-exchange-direct-execution-settle.md b/runbooks/ri-exchange-direct-execution-settle.md index 96706deef..805923d7e 100644 --- a/runbooks/ri-exchange-direct-execution-settle.md +++ b/runbooks/ri-exchange-direct-execution-settle.md @@ -26,14 +26,15 @@ Do not guess. Marking a row `failed` releases the RIs for another exchange. ## Step 2: find the audit row id The refusal hides the blocking row's id. Find it in the server log (search for -"held by direct execute"), or query by the RI id named in the refusal (`:ri`): +"held by direct execute"), or query by the RI id named in the refusal (`:'ri'`; set the psql variables first, for example +`psql -v ri=ri-0abc -v id= -v new_status=failed -v note='checked at AWS'`, or `\set id ''`): ```sql SELECT id, status, created_at FROM ri_exchange_direct_executions WHERE provider = 'aws' AND status IN ('submitted', 'unknown') AND EXISTS (SELECT 1 FROM unnest(source_ids) x - WHERE lower(btrim(x, E' \t\n\r\f\x0B')) = lower(btrim(:ri, E' \t\n\r\f\x0B'))); + WHERE lower(btrim(x, E' \t\n\r\f\x0B')) = lower(btrim(:'ri', E' \t\n\r\f\x0B'))); ``` ## Step 3: settle the row (the interim DBA step) @@ -42,12 +43,12 @@ This rewrites rows that the audit design otherwise treats as final. It is the interim until the resolve path (#1836) exists. Run only after Step 1. ```sql --- :id = the audit row id; :new_status = 'failed' (AWS shows no exchange) or 'completed' (AWS shows it happened) +-- :'id' = the audit row id; :'new_status' = 'failed' (AWS shows no exchange) or 'completed' (AWS shows it happened) UPDATE ri_exchange_direct_executions - SET status = :new_status, + SET status = :'new_status', completed_at = COALESCE(completed_at, now()), - error = concat_ws(E'\n', error, 'operator-settled ' || now()::text || ': ' || :note) - WHERE id = :id + error = concat_ws(E'\n', error, 'operator-settled ' || now()::text || ': ' || :'note') + WHERE id = :'id' AND provider = 'aws' AND status IN ('submitted', 'unknown') AND (status = 'unknown' OR created_at < now() - interval '5 minutes');