From 89537027996480966a275ce6de6df91c2e47e553 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 23:24:55 +0100 Subject: [PATCH 01/30] feat(db): add resolution columns to ri_exchange_direct_executions Adds resolved_status, resolved_by_user_id, resolved_by, resolved_at and resolution_note with an all-or-none CHECK. The original status is never rewritten. Refs #1836 Co-Authored-By: Claude Opus 5.5 --- ...163_ri_exchange_direct_resolution.down.sql | 10 ++ ...00163_ri_exchange_direct_resolution.up.sql | 24 ++++ ...0163_ri_exchange_direct_resolution_test.go | 126 ++++++++++++++++++ 3 files changed, 160 insertions(+) create mode 100644 internal/database/postgres/migrations/000163_ri_exchange_direct_resolution.down.sql create mode 100644 internal/database/postgres/migrations/000163_ri_exchange_direct_resolution.up.sql create mode 100644 internal/database/postgres/migrations/000163_ri_exchange_direct_resolution_test.go diff --git a/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution.down.sql b/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution.down.sql new file mode 100644 index 000000000..b7c75ae1e --- /dev/null +++ b/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution.down.sql @@ -0,0 +1,10 @@ +-- Reverse 000163: drop the operator-resolution columns (issue #1836). +-- Lossy by design: resolutions are dropped, so resolved rows that are still +-- submitted or unknown block their source RIs again, the fail-safe direction. +ALTER TABLE ri_exchange_direct_executions + DROP CONSTRAINT IF EXISTS ri_exchange_direct_executions_resolution_all_or_none, + DROP COLUMN IF EXISTS resolved_status, + DROP COLUMN IF EXISTS resolved_by_user_id, + DROP COLUMN IF EXISTS resolved_by, + DROP COLUMN IF EXISTS resolved_at, + DROP COLUMN IF EXISTS resolution_note; diff --git a/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution.up.sql b/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution.up.sql new file mode 100644 index 000000000..092ac4fa0 --- /dev/null +++ b/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution.up.sql @@ -0,0 +1,24 @@ +-- Operator resolution of a stuck AWS direct RI exchange row (issue #1836). +-- The original status and completed_at are never rewritten; the operator's +-- verdict is recorded beside them, and a resolved row stops holding its +-- source RIs (the guard adds `resolved_at IS NULL`). +ALTER TABLE ri_exchange_direct_executions + ADD COLUMN IF NOT EXISTS resolved_status TEXT + CONSTRAINT ri_exchange_direct_executions_resolved_status_check + CHECK (resolved_status IN ('completed', 'failed')), + ADD COLUMN IF NOT EXISTS resolved_by_user_id UUID, + ADD COLUMN IF NOT EXISTS resolved_by TEXT, + ADD COLUMN IF NOT EXISTS resolved_at TIMESTAMPTZ, + ADD COLUMN IF NOT EXISTS resolution_note TEXT; + +-- A resolution is whole or absent. resolved_by_user_id stays optional (a +-- break-glass SQL settle or an API key has no user id) but never exists +-- without a resolution. +ALTER TABLE ri_exchange_direct_executions + DROP CONSTRAINT IF EXISTS ri_exchange_direct_executions_resolution_all_or_none; +ALTER TABLE ri_exchange_direct_executions + ADD CONSTRAINT ri_exchange_direct_executions_resolution_all_or_none + CHECK ((resolved_status IS NULL) = (resolved_by IS NULL) + AND (resolved_status IS NULL) = (resolved_at IS NULL) + AND (resolved_status IS NULL) = (resolution_note IS NULL) + AND (resolved_by_user_id IS NULL OR resolved_status IS NOT NULL)); diff --git a/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution_test.go b/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution_test.go new file mode 100644 index 000000000..7f1ac67a8 --- /dev/null +++ b/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution_test.go @@ -0,0 +1,126 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// directResolutionMigration is this migration's version. A renumber at merge +// changes this constant and the two file names, nothing else. +const directResolutionMigration uint = 163 + +var directResolutionColumns = []string{"resolved_status", "resolved_by_user_id", "resolved_by", "resolved_at", "resolution_note"} + +func directResolutionColumnState(t *testing.T, ctx context.Context, pool *pgxpool.Pool) map[string]bool { + t.Helper() + rows, err := pool.Query(ctx, ` + SELECT column_name, is_nullable = 'YES' FROM information_schema.columns + WHERE table_name = 'ri_exchange_direct_executions' AND column_name = ANY($1)`, directResolutionColumns) + require.NoError(t, err) + defer rows.Close() + nullable := map[string]bool{} + for rows.Next() { + var name string + var isNullable bool + require.NoError(t, rows.Scan(&name, &isNullable)) + nullable[name] = isNullable + } + require.NoError(t, rows.Err()) + return nullable +} + +// insertDirectRow inserts an unknown AWS row with the given extra columns and +// their SQL literal values. +func insertDirectRow(ctx context.Context, pool *pgxpool.Pool, cols, vals string) error { + _, err := pool.Exec(ctx, ` + INSERT INTO ri_exchange_direct_executions + (provider, source_ids, targets, max_payment_due, max_payment_currency, executed_by, status, completed_at`+cols+`) + VALUES ('aws', ARRAY['ri-1'], '[{"offering_id":"o"}]'::jsonb, 100, 'USD', 'alice@example.com', 'unknown', now()`+vals+`)`) + return err +} + +// TestMigration_RIExchangeDirectResolution pins the #1836 migration against a +// real PostgreSQL: five nullable columns appear, a resolution must be whole or +// absent, resolved_status is limited to completed or failed, the up statement +// is idempotent, and the down migration drops the columns. +func TestMigration_RIExchangeDirectResolution(t *testing.T) { + t.Parallel() + ctx := context.Background() + migrationsPath := getMigrationsPath() + + container := testhelpers.RequireEmptyDatabase(ctx, t) + pool := container.DB.Pool() + + prev := previousMigrationVersion(t, directResolutionMigration) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, prev)) + require.Empty(t, directResolutionColumnState(t, ctx, pool), "columns must not exist before the migration") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, directResolutionMigration)) + state := directResolutionColumnState(t, ctx, pool) + require.Len(t, state, len(directResolutionColumns)) + for _, c := range directResolutionColumns { + assert.True(t, state[c], "%s must be nullable", c) + } + + const whole = ", resolved_status, resolved_by, resolved_at, resolution_note" + // An unresolved row (all NULL) and whole resolutions are accepted. + require.NoError(t, insertDirectRow(ctx, pool, "", "")) + require.NoError(t, insertDirectRow(ctx, pool, whole, ", 'failed', 'ops', now(), 'note'")) + require.NoError(t, insertDirectRow(ctx, pool, whole+", resolved_by_user_id", + ", 'completed', 'ops', now(), 'note', '00000000-0000-0000-0000-000000000001'")) + + // Each partial resolution is refused. + for name, c := range map[string]struct{ cols, vals string }{ + "status only": {", resolved_status", ", 'failed'"}, + "no note": {", resolved_status, resolved_by, resolved_at", ", 'failed', 'ops', now()"}, + "no resolved_at": {", resolved_status, resolved_by, resolution_note", ", 'failed', 'ops', 'n'"}, + "no resolved_by": {", resolved_status, resolved_at, resolution_note", ", 'failed', now(), 'n'"}, + "no status": {", resolved_by, resolved_at, resolution_note", ", 'ops', now(), 'n'"}, + "note only": {", resolution_note", ", 'n'"}, + "user id only (no resolution)": {", resolved_by_user_id", ", '00000000-0000-0000-0000-000000000001'"}, + } { + err := insertDirectRow(ctx, pool, c.cols, c.vals) + require.Error(t, err, name) + assert.Contains(t, err.Error(), "ri_exchange_direct_executions_resolution_all_or_none", name) + } + + // A whole resolution with a verdict outside completed/failed is refused. + for _, bad := range []string{"unknown", "submitted", ""} { + err := insertDirectRow(ctx, pool, whole, fmt.Sprintf(", '%s', 'ops', now(), 'n'", bad)) + require.Error(t, err, "resolved_status %q", bad) + assert.Contains(t, err.Error(), "ri_exchange_direct_executions_resolved_status_check", "resolved_status %q", bad) + } + + up, err := os.ReadFile(filepath.Join(migrationsPath, fmt.Sprintf("%06d_ri_exchange_direct_resolution.up.sql", directResolutionMigration))) + require.NoError(t, err) + _, err = pool.Exec(ctx, string(up)) + require.NoError(t, err, "the up statement must be idempotent") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, prev)) + assert.Empty(t, directResolutionColumnState(t, ctx, pool), "down migration must drop the columns") + var rows int + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM ri_exchange_direct_executions`).Scan(&rows)) + assert.Equal(t, 3, rows, "down is lossy for the columns only, never the rows") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, directResolutionMigration)) + state = directResolutionColumnState(t, ctx, pool) + require.Len(t, state, len(directResolutionColumns), "up again restores the columns") + err = insertDirectRow(ctx, pool, ", resolved_status", ", 'failed'") + require.Error(t, err, "up again restores the all-or-none CHECK") + assert.Contains(t, err.Error(), "ri_exchange_direct_executions_resolution_all_or_none") + err = insertDirectRow(ctx, pool, whole, ", 'unknown', 'ops', now(), 'n'") + require.Error(t, err, "up again restores the enum CHECK") + assert.Contains(t, err.Error(), "ri_exchange_direct_executions_resolved_status_check") +} From b8a3a1e4b5913b04044fdb3fd46d4f88a6f97092 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 23:25:13 +0100 Subject: [PATCH 02/30] fix(ri-exchange): release resolved direct rows from the source guard The direct-execution guard skips rows with resolved_at set, so an operator-resolved row no longer holds its source RIs. Refs #1836 Co-Authored-By: Claude Opus 5.5 --- internal/config/store_postgres.go | 7 +- ..._direct_resolved_guard_integration_test.go | 125 ++++++++++++++++++ 2 files changed, 129 insertions(+), 3 deletions(-) create mode 100644 internal/config/store_postgres_ri_exchange_direct_resolved_guard_integration_test.go diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 6e13cb0d4..4a8d85604 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -3488,8 +3488,9 @@ func normaliseRIExchangeSourceIDs(sources []string) []string { // 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. +// source RIs until an operator resolves it (resolved_at set, #1836). 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). @@ -3505,7 +3506,7 @@ func refuseRIExchangeSourceInFlightDirect(ctx context.Context, tx pgx.Tx, source 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') + WHERE provider = 'aws' AND status IN ('submitted', 'unknown') AND resolved_at IS NULL ) d WHERE ids && $1::text[] ORDER BY created_at, id diff --git a/internal/config/store_postgres_ri_exchange_direct_resolved_guard_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_resolved_guard_integration_test.go new file mode 100644 index 000000000..0b449947b --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_resolved_guard_integration_test.go @@ -0,0 +1,125 @@ +//go:build integration + +package config + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Real-PostgreSQL coverage for #1836: an operator-resolved direct row no +// longer holds its source RIs, on all three guarded paths; an unresolved one +// still does. + +// resolveDirectRow stamps the resolution columns directly, as the resolve +// store method will, without changing status or completed_at. +func (e *riExchangeIntegrationEnv) resolveDirectRow(t *testing.T, id, resolvedStatus string) { + t.Helper() + tag, err := e.poolA.Exec(e.ctx, ` + UPDATE ri_exchange_direct_executions + SET resolved_status = $2, resolved_by = 'operator', resolved_at = now(), resolution_note = 'checked at AWS' + WHERE id = $1`, id, resolvedStatus) + require.NoError(t, err) + require.EqualValues(t, 1, tag.RowsAffected()) +} + +// [fail-first] A resolved submitted or unknown row stops refusing an AUTO +// reservation, while an unresolved row of the same status still refuses. +func TestPostgresStoreDB_ReserveRIExchange_ResolvedDirectRowDoesNotBlock(t *testing.T) { + for _, status := range []string{"submitted", "unknown"} { + for _, resolved := range []string{"completed", "failed"} { + t.Run(status+"_"+resolved, func(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + id := env.directRow(t, "aws", status, "ri-g1836-res") + env.directRow(t, "aws", status, "ri-g1836-open") + + err := env.reserve(t, testAccountID(836000000001), "ri-g1836-res") + require.ErrorIs(t, err, ErrRIExchangeSourceInFlight, "unresolved row blocks") + + env.resolveDirectRow(t, id, resolved) + require.NoError(t, env.reserve(t, testAccountID(836000000002), "ri-g1836-res"), "resolved row releases its RI") + require.ErrorIs(t, env.reserve(t, testAccountID(836000000003), "ri-g1836-open"), ErrRIExchangeSourceInFlight, "another unresolved row still blocks") + }) + } + } +} + +// [fail-first] The approval path releases a resolved row's RI too, and the +// refusal text for a still-unresolved row never names a resolved row. +func TestPostgresStoreDB_ReserveApprovedRIExchange_ResolvedDirectRowDoesNotBlock(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + cap := env.capAbove(t, time.Now().UTC()) + resolved := env.directRow(t, "aws", "unknown", "ri-g1836-appr-res", "ri-g1836-appr-shared") + open := env.directRow(t, "aws", "unknown", "ri-g1836-appr-open", "ri-g1836-appr-shared") + + row := env.processingWith(t, testAccountID(836000000011), "ri-g1836-appr-res", "ri-g1836-appr-open") + defer env.release(t, row.ID) + _, err := env.storeA.ReserveApprovedRIExchange(env.ctx, row.ID, cap, cap) + require.ErrorIs(t, err, ErrRIExchangeSourceInFlight, "both rows unresolved") + + env.resolveDirectRow(t, resolved, "completed") + _, err = env.storeA.ReserveApprovedRIExchange(env.ctx, row.ID, cap, cap) + require.ErrorIs(t, err, ErrRIExchangeSourceInFlight, "the open row still blocks") + assert.Contains(t, err.Error(), "ri-g1836-appr-open") + assert.NotContains(t, err.Error(), "ri-g1836-appr-res", "a resolved row's RI is not named") + assert.NotContains(t, err.Error(), open) + assert.NotContains(t, err.Error(), resolved) + + env.resolveDirectRow(t, open, "failed") + _, err = env.storeA.ReserveApprovedRIExchange(env.ctx, row.ID, cap, cap) + require.NoError(t, err, "every holder resolved") +} + +// [fail-first] The direct Save admits a source RI whose holder was resolved, +// and still refuses one whose holder is unresolved. +func TestRIExchangeDirectSave_ResolvedHolderDoesNotBlock(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + held := awsDirectSave("ri-g1836-save") + require.NoError(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, held)) + require.ErrorIs(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave("ri-g1836-save")), ErrRIExchangeSourceInFlight, "unresolved holder") + + env.resolveDirectRow(t, held.ID, "failed") + require.NoError(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave("ri-g1836-save")), "resolved holder releases the RI") + require.ErrorIs(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave("ri-g1836-save")), ErrRIExchangeSourceInFlight, "the new submitted row holds it again") +} + +// [fail-first] The full matrix on the approval path: a submitted or unknown +// row, resolved as completed or as failed, stops blocking; unresolved blocks. +func TestPostgresStoreDB_ReserveApprovedRIExchange_ResolvedMatrix(t *testing.T) { + for _, status := range []string{"submitted", "unknown"} { + for _, resolved := range []string{"completed", "failed"} { + t.Run(status+"_"+resolved, func(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + cap := env.capAbove(t, time.Now().UTC()) + id := env.directRow(t, "aws", status, "ri-g1836-mx") + row := env.processingWith(t, testAccountID(836000000021), "ri-g1836-mx") + defer env.release(t, row.ID) + + _, err := env.storeA.ReserveApprovedRIExchange(env.ctx, row.ID, cap, cap) + require.ErrorIs(t, err, ErrRIExchangeSourceInFlight, "unresolved row blocks") + env.resolveDirectRow(t, id, resolved) + _, err = env.storeA.ReserveApprovedRIExchange(env.ctx, row.ID, cap, cap) + require.NoError(t, err, "resolved row releases its RI") + }) + } + } +} + +// [fail-first] The full matrix on the direct Save path. +func TestRIExchangeDirectSave_ResolvedMatrix(t *testing.T) { + for _, status := range []string{"submitted", "unknown"} { + for _, resolved := range []string{"completed", "failed"} { + t.Run(status+"_"+resolved, func(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + id := env.directRow(t, "aws", status, "ri-g1836-sv") + + require.ErrorIs(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave("ri-g1836-sv")), ErrRIExchangeSourceInFlight, "unresolved row blocks") + env.resolveDirectRow(t, id, resolved) + require.NoError(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave("ri-g1836-sv")), "resolved row releases its RI") + }) + } + } +} From 74fbd41201c49147bf32870500e2e734a524498a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 23:42:05 +0100 Subject: [PATCH 03/30] feat(config): list unresolved AWS direct RI-exchange rows Keyset-paged read of submitted and unknown AWS rows that no operator has resolved, restricted to an allowed set of (account, region) pairs passed as two arrays. Filters: status, region and a normalised RI id. Refs #1836 Co-Authored-By: Claude Opus 5.5 --- internal/config/ri_exchange_direct_list.go | 57 +++++ .../store_postgres_ri_exchange_direct_list.go | 121 +++++++++++ ...i_exchange_direct_list_integration_test.go | 201 ++++++++++++++++++ 3 files changed, 379 insertions(+) create mode 100644 internal/config/ri_exchange_direct_list.go create mode 100644 internal/config/store_postgres_ri_exchange_direct_list.go create mode 100644 internal/config/store_postgres_ri_exchange_direct_list_integration_test.go diff --git a/internal/config/ri_exchange_direct_list.go b/internal/config/ri_exchange_direct_list.go new file mode 100644 index 000000000..3043fb571 --- /dev/null +++ b/internal/config/ri_exchange_direct_list.go @@ -0,0 +1,57 @@ +package config + +import ( + "encoding/json" + "time" +) + +// RIExchangeDirectRow is an AWS direct-execute audit row as the operator +// resolve API reads it. It carries no executing user id or API key id. Error +// is the raw stored provider text: callers pass it through +// purchase.SanitizeStoredError and never return it as is. +type RIExchangeDirectRow struct { + ID string + Status string + CloudAccountID *string + Region string + SourceIDs []string + Targets json.RawMessage + MaxPaymentDue string + MaxPaymentCurrency string + Error string + ExecutedBy string + CreatedAt time.Time + CompletedAt *time.Time + ResolvedAt *time.Time +} + +// RIExchangeDirectAccountRegion is one (account, region) pair of a row. A nil +// CloudAccountID is a row whose deployment mapped to no registered account. +type RIExchangeDirectAccountRegion struct { + CloudAccountID *string + Region string +} + +// RIExchangeDirectCursor is the keyset position after the last row of a page. +type RIExchangeDirectCursor struct { + CreatedAt time.Time + ID string +} + +// RIExchangeDirectListFilter selects unresolved AWS direct rows. Allowed is +// the set of (account, region) pairs the caller may see, derived by the +// handler from the caller's own authorization; an empty set returns nothing. +type RIExchangeDirectListFilter struct { + Allowed []RIExchangeDirectAccountRegion + Statuses []string // submitted and/or unknown; empty means both + RIID string // matched like the in-flight guard: trimmed, lower case + Region string + Limit int + After *RIExchangeDirectCursor +} + +// RIExchangeDirectListPage is one page, oldest first. Next is nil at the end. +type RIExchangeDirectListPage struct { + Rows []RIExchangeDirectRow + Next *RIExchangeDirectCursor +} diff --git a/internal/config/store_postgres_ri_exchange_direct_list.go b/internal/config/store_postgres_ri_exchange_direct_list.go new file mode 100644 index 000000000..ed18ddc99 --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_list.go @@ -0,0 +1,121 @@ +package config + +import ( + "context" + "fmt" + "time" + + "github.com/jackc/pgx/v5" +) + +// The list's page size: the default, and the most a caller may ask for. +const ( + RIExchangeDirectListDefaultLimit = 50 + RIExchangeDirectListMaxLimit = 200 +) + +const riExchangeDirectRowColumns = ` + id::text, status, cloud_account_id, COALESCE(region, ''), source_ids, targets::text, + max_payment_due::text, max_payment_currency, COALESCE(error, ''), executed_by, + created_at, completed_at, resolved_at` + +func scanRIExchangeDirectRow(row pgx.Row) (RIExchangeDirectRow, error) { + var r RIExchangeDirectRow + var targets string + err := row.Scan(&r.ID, &r.Status, &r.CloudAccountID, &r.Region, &r.SourceIDs, &targets, + &r.MaxPaymentDue, &r.MaxPaymentCurrency, &r.Error, &r.ExecutedBy, + &r.CreatedAt, &r.CompletedAt, &r.ResolvedAt) + r.Targets = []byte(targets) + return r, err +} + +// ListUnresolvedRIExchangeDirectExecutions returns one keyset page of +// unresolved AWS direct rows (submitted or unknown), oldest first, restricted +// to the filter's allowed (account, region) pairs. The pairs travel as two +// arrays so no caller input reaches the SQL text, a NULL account matches only +// a NULL pair entry, and a NULL region matches the empty region. +func (s *PostgresStore) ListUnresolvedRIExchangeDirectExecutions(ctx context.Context, f RIExchangeDirectListFilter) (*RIExchangeDirectListPage, error) { + limit, statuses, err := validateRIExchangeDirectListFilter(f) + if err != nil { + return nil, err + } + page := &RIExchangeDirectListPage{} + if len(f.Allowed) == 0 { + return page, nil + } + accounts := make([]*string, len(f.Allowed)) + regions := make([]string, len(f.Allowed)) + for i, p := range f.Allowed { + accounts[i], regions[i] = p.CloudAccountID, p.Region + } + riID, region := optionalListText(normaliseRIExchangeSourceIDs([]string{f.RIID})), optionalListText([]string{f.Region}) + var afterAt *time.Time + var afterID *string + if f.After != nil { + afterAt, afterID = &f.After.CreatedAt, &f.After.ID + } + + rows, err := s.db.Query(ctx, ` + SELECT`+riExchangeDirectRowColumns+` + FROM ri_exchange_direct_executions t + WHERE provider = 'aws' AND resolved_at IS NULL + AND status = ANY($1::text[]) + AND EXISTS (SELECT 1 FROM unnest($2::text[], $3::text[]) a(acc, reg) + WHERE t.cloud_account_id IS NOT DISTINCT FROM a.acc AND COALESCE(t.region, '') = a.reg) + AND ($4::text IS NULL OR $4 = ANY(ARRAY(SELECT lower(btrim(x, E' \t\n\r\f\x0B')) FROM unnest(source_ids) x))) + AND ($5::text IS NULL OR region = $5) + AND ($6::timestamptz IS NULL OR (created_at, id) > ($6, $7::uuid)) + ORDER BY created_at, id + LIMIT $8`, + statuses, accounts, regions, riID, region, afterAt, afterID, limit+1) + if err != nil { + return nil, fmt.Errorf("failed to list unresolved ri exchange direct executions: %w", err) + } + defer rows.Close() + for rows.Next() { + r, err := scanRIExchangeDirectRow(rows) + if err != nil { + return nil, fmt.Errorf("failed to scan ri exchange direct execution: %w", err) + } + page.Rows = append(page.Rows, r) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("failed to list unresolved ri exchange direct executions: %w", err) + } + if len(page.Rows) > limit { + page.Rows = page.Rows[:limit] + last := page.Rows[limit-1] + page.Next = &RIExchangeDirectCursor{CreatedAt: last.CreatedAt, ID: last.ID} + } + return page, nil +} + +// validateRIExchangeDirectListFilter applies the limit default and cap and +// checks the status filter. +func validateRIExchangeDirectListFilter(f RIExchangeDirectListFilter) (int, []string, error) { + limit := f.Limit + if limit <= 0 { + limit = RIExchangeDirectListDefaultLimit + } + if limit > RIExchangeDirectListMaxLimit { + return 0, nil, fmt.Errorf("%w: limit must be at most %d", ErrRIExchangeResolveInvalid, RIExchangeDirectListMaxLimit) + } + statuses := f.Statuses + if len(statuses) == 0 { + statuses = []string{RIExchangeDirectStatusSubmitted, RIExchangeDirectStatusUnknown} + } + for _, st := range statuses { + if st != RIExchangeDirectStatusSubmitted && st != RIExchangeDirectStatusUnknown { + return 0, nil, fmt.Errorf("%w: status must be %q or %q", ErrRIExchangeResolveInvalid, RIExchangeDirectStatusSubmitted, RIExchangeDirectStatusUnknown) + } + } + return limit, statuses, nil +} + +// optionalListText is the first non-empty value as a nullable SQL argument. +func optionalListText(vals []string) *string { + if len(vals) == 0 || vals[0] == "" { + return nil + } + return &vals[0] +} diff --git a/internal/config/store_postgres_ri_exchange_direct_list_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_list_integration_test.go new file mode 100644 index 000000000..0d6e319f4 --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_list_integration_test.go @@ -0,0 +1,201 @@ +//go:build integration + +package config + +import ( + "sort" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Real-PostgreSQL coverage for the #1836 list and get reads. + +// directSpec describes one audit row for the list tests. +type directSpec struct { + provider, status string + account *string + region string + sources []string + createdAt time.Time + nullRegion bool + resolved bool + errText string +} + +// insertDirect writes an audit row exactly as described and returns its id. +func (e *riExchangeIntegrationEnv) insertDirect(t *testing.T, s directSpec) string { + t.Helper() + if s.provider == "" { + s.provider = "aws" + } + if s.status == "" { + s.status = "unknown" + } + var region *string + if s.region != "" || (s.provider == "aws" && !s.nullRegion) { + if s.region == "" { + s.region = "eu-west-1" + } + region = &s.region + } + if len(s.sources) == 0 { + s.sources = []string{"ri-" + uuid.NewString()} + } + if s.createdAt.IsZero() { + s.createdAt = time.Now().UTC().Truncate(time.Microsecond) + } + var errText *string + if s.errText != "" { + errText = &s.errText + } + var id string + require.NoError(t, e.poolA.QueryRow(e.ctx, ` + INSERT INTO ri_exchange_direct_executions + (provider, cloud_account_id, region, source_ids, targets, max_payment_due, max_payment_currency, + status, error, executed_by, created_at, completed_at, + resolved_status, resolved_by, resolved_at, resolution_note) + VALUES ($1, $2, $3, $4, '[{"offering_id":"o","count":1}]'::jsonb, 100, 'USD', + $5, $6, 'tester', $7, CASE WHEN $5 = 'submitted' THEN NULL ELSE $7::timestamptz END, + CASE WHEN $8 THEN 'failed' END, CASE WHEN $8 THEN 'op' END, CASE WHEN $8 THEN now() END, CASE WHEN $8 THEN 'note' END) + RETURNING id::text`, + s.provider, s.account, region, s.sources, s.status, errText, s.createdAt, s.resolved).Scan(&id)) + return id +} + +func rowIDs(rows []RIExchangeDirectRow) []string { + ids := make([]string, len(rows)) + for i, r := range rows { + ids[i] = r.ID + } + return ids +} + +func allow(account *string, region string) []RIExchangeDirectAccountRegion { + return []RIExchangeDirectAccountRegion{{CloudAccountID: account, Region: region}} +} + +// [fail-first] Only unresolved submitted and unknown AWS rows are listed. +func TestPostgresStoreDB_RIExchangeDirectList_OnlyUnresolvedAWSInFlight(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + acc := strPtr("acc-1") + want := []string{ + env.insertDirect(t, directSpec{status: "submitted", account: acc}), + env.insertDirect(t, directSpec{status: "unknown", account: acc}), + } + env.insertDirect(t, directSpec{provider: "azure", status: "unknown", account: acc, region: "eu-west-1"}) // same account and region, so only the provider predicate excludes it + env.insertDirect(t, directSpec{status: "completed", account: acc}) + env.insertDirect(t, directSpec{status: "failed", account: acc}) + env.insertDirect(t, directSpec{status: "unknown", account: acc, resolved: true}) + env.insertDirect(t, directSpec{status: "submitted", account: acc, resolved: true}) + + page, err := env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, RIExchangeDirectListFilter{Allowed: allow(acc, "eu-west-1")}) + require.NoError(t, err) + assert.ElementsMatch(t, want, rowIDs(page.Rows)) + assert.Nil(t, page.Next) +} + +// [fail-first] The status, region and normalised ri_id filters. +func TestPostgresStoreDB_RIExchangeDirectList_Filters(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + acc := strPtr("acc-1") + sub := env.insertDirect(t, directSpec{status: "submitted", account: acc, sources: []string{"ri-a"}}) + unk := env.insertDirect(t, directSpec{status: "unknown", account: acc, sources: []string{" RI-PADDED ", "ri-b"}}) + other := env.insertDirect(t, directSpec{status: "unknown", account: acc, region: "us-east-1", sources: []string{"ri-c"}}) + allowed := []RIExchangeDirectAccountRegion{{acc, "eu-west-1"}, {acc, "us-east-1"}} + list := func(f RIExchangeDirectListFilter) []string { + f.Allowed = allowed + page, err := env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, f) + require.NoError(t, err) + ids := rowIDs(page.Rows) + sort.Strings(ids) + return ids + } + sorted := func(ids ...string) []string { sort.Strings(ids); return ids } + + assert.Equal(t, sorted(sub, unk, other), list(RIExchangeDirectListFilter{}), "default is both statuses") + assert.Equal(t, []string{sub}, list(RIExchangeDirectListFilter{Statuses: []string{"submitted"}})) + assert.Equal(t, sorted(unk, other), list(RIExchangeDirectListFilter{Statuses: []string{"unknown"}})) + assert.Equal(t, []string{other}, list(RIExchangeDirectListFilter{Region: "us-east-1"})) + assert.Equal(t, []string{unk}, list(RIExchangeDirectListFilter{RIID: "ri-padded"}), "stored padded upper case matches") + assert.Equal(t, []string{unk}, list(RIExchangeDirectListFilter{RIID: " Ri-B "}), "padded mixed-case request matches") + assert.Empty(t, list(RIExchangeDirectListFilter{RIID: "ri-none"})) + + _, err := env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, RIExchangeDirectListFilter{Allowed: allowed, Statuses: []string{"completed"}}) + require.ErrorIs(t, err, ErrRIExchangeResolveInvalid) + _, err = env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, RIExchangeDirectListFilter{Allowed: allowed, Limit: 201}) + require.ErrorIs(t, err, ErrRIExchangeResolveInvalid) +} + +// [fail-first] The allowed (account, region) set bounds the page, a NULL +// account matches only a NULL entry, and an empty set lists nothing. +func TestPostgresStoreDB_RIExchangeDirectList_AllowedAccountRegionSet(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + a1, a2 := strPtr("acc-1"), strPtr("acc-2") + r1 := env.insertDirect(t, directSpec{account: a1}) + r2 := env.insertDirect(t, directSpec{account: a2}) + rOtherRegion := env.insertDirect(t, directSpec{account: a1, region: "us-east-1"}) + rNull := env.insertDirect(t, directSpec{account: nil}) + list := func(allowed []RIExchangeDirectAccountRegion) []string { + page, err := env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, RIExchangeDirectListFilter{Allowed: allowed}) + require.NoError(t, err) + ids := rowIDs(page.Rows) + sort.Strings(ids) + return ids + } + sorted := func(ids ...string) []string { sort.Strings(ids); return ids } + + assert.Equal(t, []string{r1}, list(allow(a1, "eu-west-1")), "one account, one region") + assert.Equal(t, sorted(r1, r2), list([]RIExchangeDirectAccountRegion{{a1, "eu-west-1"}, {a2, "eu-west-1"}})) + assert.Equal(t, sorted(r1, rOtherRegion), list([]RIExchangeDirectAccountRegion{{a1, "eu-west-1"}, {a1, "us-east-1"}})) + assert.Equal(t, []string{rNull}, list(allow(nil, "eu-west-1")), "a NULL account is listed only for a NULL entry") + assert.Empty(t, list(allow(strPtr("acc-3"), "eu-west-1"))) + assert.Empty(t, list(allow(a1, "ap-south-1")), "right account, wrong region") + assert.Empty(t, list(nil), "an empty set lists nothing") +} + +// [fail-first] Keyset pages are complete and ordered with equal created_at. +func TestPostgresStoreDB_RIExchangeDirectList_KeysetPagination(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + acc := strPtr("acc-1") + base := time.Now().UTC().Truncate(time.Microsecond).Add(-time.Hour) + var all []string + for _, at := range []time.Time{base, base, base, base, base.Add(time.Second), base.Add(time.Second), base.Add(2 * time.Second)} { + all = append(all, env.insertDirect(t, directSpec{account: acc, createdAt: at})) + } + type key struct { + at time.Time + id string + } + var keys []key + full, err := env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, RIExchangeDirectListFilter{Allowed: allow(acc, "eu-west-1")}) + require.NoError(t, err) + for _, r := range full.Rows { + keys = append(keys, key{r.CreatedAt, r.ID}) + } + require.Len(t, keys, len(all)) + require.True(t, sort.SliceIsSorted(keys, func(i, j int) bool { + if !keys[i].at.Equal(keys[j].at) { + return keys[i].at.Before(keys[j].at) + } + return keys[i].id < keys[j].id + }), "oldest first, id breaks ties") + + var got []string + var after *RIExchangeDirectCursor + for pages := 0; pages < 10; pages++ { + page, err := env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, + RIExchangeDirectListFilter{Allowed: allow(acc, "eu-west-1"), Limit: 2, After: after}) + require.NoError(t, err) + got = append(got, rowIDs(page.Rows)...) + if page.Next == nil { + break + } + require.Len(t, page.Rows, 2) + after = page.Next + } + assert.Equal(t, rowIDs(full.Rows), got, "pages concatenate to the full ordered list with no gap or repeat") +} From 0c0c6e5a498d80cdedc8b78ed15500ea55ce538c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 8 Nov 2026 10:25:14 +0100 Subject: [PATCH 04/30] test(config): pin NULL-region listing and early filter validation The NULL-region row is listed for the empty region, and a bad filter is invalid even when nothing is allowed. Refs #1836 Co-Authored-By: Claude Opus 5.5 --- ...i_exchange_direct_list_integration_test.go | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/internal/config/store_postgres_ri_exchange_direct_list_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_list_integration_test.go index 0d6e319f4..9377b547f 100644 --- a/internal/config/store_postgres_ri_exchange_direct_list_integration_test.go +++ b/internal/config/store_postgres_ri_exchange_direct_list_integration_test.go @@ -199,3 +199,30 @@ func TestPostgresStoreDB_RIExchangeDirectList_KeysetPagination(t *testing.T) { } assert.Equal(t, rowIDs(full.Rows), got, "pages concatenate to the full ordered list with no gap or repeat") } + +// [fail-first] An AWS row with a NULL region is listed for the empty region, +// the same pair the pair read reports for it. +func TestPostgresStoreDB_RIExchangeDirectList_NullRegionRowIsListable(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + acc := strPtr("acc-1") + id := env.insertDirect(t, directSpec{account: acc, nullRegion: true}) + env.insertDirect(t, directSpec{account: acc}) + + page, err := env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, RIExchangeDirectListFilter{Allowed: allow(acc, "")}) + require.NoError(t, err) + require.Equal(t, []string{id}, rowIDs(page.Rows)) + assert.Equal(t, "", page.Rows[0].Region) +} + +// [fail-first] A bad filter is invalid even when nothing is allowed, never an +// empty result. +func TestPostgresStoreDB_RIExchangeDirectList_BadFilterIsInvalidWithEmptyAllowSet(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + for name, f := range map[string]RIExchangeDirectListFilter{ + "limit": {Limit: 201}, + "status": {Statuses: []string{"completed"}}, + } { + _, err := env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, f) + require.ErrorIs(t, err, ErrRIExchangeResolveInvalid, name) + } +} From d5aafe3477c38979c9f9e291850ce3f92c7d6d71 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 23:43:38 +0100 Subject: [PATCH 05/30] feat(config): read one AWS direct RI-exchange row and its account pairs GetRIExchangeDirectExecution returns an AWS row of any status for the resolve handler to authorize against. The distinct unresolved (account, region) pairs let the handler ask the authorization service once per pair. Refs #1836 Co-Authored-By: Claude Opus 5.5 --- .../store_postgres_ri_exchange_direct_get.go | 60 ++++++++++++++++ ...ri_exchange_direct_get_integration_test.go | 69 +++++++++++++++++++ 2 files changed, 129 insertions(+) create mode 100644 internal/config/store_postgres_ri_exchange_direct_get.go create mode 100644 internal/config/store_postgres_ri_exchange_direct_get_integration_test.go diff --git a/internal/config/store_postgres_ri_exchange_direct_get.go b/internal/config/store_postgres_ri_exchange_direct_get.go new file mode 100644 index 000000000..dcd7bebd0 --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_get.go @@ -0,0 +1,60 @@ +package config + +import ( + "context" + "errors" + "fmt" + + "github.com/jackc/pgx/v5" +) + +// ListUnresolvedRIExchangeDirectAccountRegions returns the distinct (account, +// region) pairs of unresolved AWS rows, so the handler can ask the +// authorization service about each pair once. The unresolved partial index +// serves it. +func (s *PostgresStore) ListUnresolvedRIExchangeDirectAccountRegions(ctx context.Context) ([]RIExchangeDirectAccountRegion, error) { + rows, err := s.db.Query(ctx, ` + SELECT DISTINCT cloud_account_id, COALESCE(region, '') + FROM ri_exchange_direct_executions + WHERE provider = 'aws' AND status IN ('submitted', 'unknown') AND resolved_at IS NULL + ORDER BY 1 NULLS FIRST, 2`) + if err != nil { + return nil, fmt.Errorf("failed to list unresolved ri exchange direct accounts: %w", err) + } + defer rows.Close() + var out []RIExchangeDirectAccountRegion + for rows.Next() { + var p RIExchangeDirectAccountRegion + if err := rows.Scan(&p.CloudAccountID, &p.Region); err != nil { + return nil, fmt.Errorf("failed to scan unresolved ri exchange direct account: %w", err) + } + out = append(out, p) + } + return out, rows.Err() +} + +// GetRIExchangeDirectExecution returns one AWS direct row of any status, or +// ErrRIExchangeResolveNotFound for a missing or non-AWS row. +func (s *PostgresStore) GetRIExchangeDirectExecution(ctx context.Context, id string) (*RIExchangeDirectRow, error) { + r, err := scanRIExchangeDirectRow(s.db.QueryRow(ctx, ` + SELECT`+riExchangeDirectRowColumns+` + FROM ri_exchange_direct_executions + WHERE id = $1 AND provider = 'aws'`, id)) + if errors.Is(err, pgx.ErrNoRows) { + return nil, ErrRIExchangeResolveNotFound + } + if err != nil { + return nil, fmt.Errorf("failed to read ri exchange direct execution %s: %w", id, err) + } + return &r, nil +} + +// RIExchangeDirectListStore is the read side of the direct-row resolve API. +// Errors are the ErrRIExchangeResolve* values. +type RIExchangeDirectListStore interface { + ListUnresolvedRIExchangeDirectAccountRegions(ctx context.Context) ([]RIExchangeDirectAccountRegion, error) + ListUnresolvedRIExchangeDirectExecutions(ctx context.Context, f RIExchangeDirectListFilter) (*RIExchangeDirectListPage, error) + GetRIExchangeDirectExecution(ctx context.Context, id string) (*RIExchangeDirectRow, error) +} + +var _ RIExchangeDirectListStore = (*PostgresStore)(nil) diff --git a/internal/config/store_postgres_ri_exchange_direct_get_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_get_integration_test.go new file mode 100644 index 000000000..02267d500 --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_get_integration_test.go @@ -0,0 +1,69 @@ +//go:build integration + +package config + +import ( + "reflect" + "strings" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// [fail-first] The distinct pairs read behind the handler's authorization. +func TestPostgresStoreDB_RIExchangeDirectList_AccountRegionPairs(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + a1 := strPtr("acc-1") + env.insertDirect(t, directSpec{account: a1}) + env.insertDirect(t, directSpec{account: a1, status: "submitted"}) + env.insertDirect(t, directSpec{account: nil, region: "us-east-1"}) + env.insertDirect(t, directSpec{account: a1, resolved: true, region: "ap-south-1"}) + env.insertDirect(t, directSpec{provider: "azure", account: a1}) + env.insertDirect(t, directSpec{status: "completed", account: strPtr("acc-9"), region: "sa-east-1"}) + + pairs, err := env.storeA.ListUnresolvedRIExchangeDirectAccountRegions(env.ctx) + require.NoError(t, err) + require.Len(t, pairs, 2) + assert.Nil(t, pairs[0].CloudAccountID) + assert.Equal(t, "us-east-1", pairs[0].Region) + assert.Equal(t, "acc-1", *pairs[1].CloudAccountID) + assert.Equal(t, "eu-west-1", pairs[1].Region) +} + +// [fail-first] Get returns an AWS row of any status, and not found for Azure or +// a missing id. +func TestPostgresStoreDB_GetRIExchangeDirectExecution(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + acc := strPtr("acc-1") + resolved := env.insertDirect(t, directSpec{account: acc, resolved: true, errText: "raw provider text"}) + az := env.insertDirect(t, directSpec{provider: "azure"}) + + got, err := env.storeA.GetRIExchangeDirectExecution(env.ctx, resolved) + require.NoError(t, err) + assert.Equal(t, resolved, got.ID) + assert.Equal(t, "unknown", got.Status) + assert.Equal(t, "eu-west-1", got.Region) + assert.Equal(t, "acc-1", *got.CloudAccountID) + assert.Equal(t, "100", got.MaxPaymentDue) + assert.Equal(t, "raw provider text", got.Error) + assert.NotNil(t, got.ResolvedAt) + assert.NotNil(t, got.CompletedAt) + assert.JSONEq(t, `[{"offering_id":"o","count":1}]`, string(got.Targets)) + + _, err = env.storeA.GetRIExchangeDirectExecution(env.ctx, az) + require.ErrorIs(t, err, ErrRIExchangeResolveNotFound) + _, err = env.storeA.GetRIExchangeDirectExecution(env.ctx, uuid.NewString()) + require.ErrorIs(t, err, ErrRIExchangeResolveNotFound) +} + +// The read type never carries who executed through which credential. +func TestRIExchangeDirectRow_HasNoCredentialFields(t *testing.T) { + typ := reflect.TypeOf(RIExchangeDirectRow{}) + for i := 0; i < typ.NumField(); i++ { + name := strings.ToLower(typ.Field(i).Name) + assert.NotContains(t, name, "userid", typ.Field(i).Name) + assert.NotContains(t, name, "apikey", typ.Field(i).Name) + } +} From 3c02b06a42fd846a46cda9cf945ee7dddf436f2c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 23:45:34 +0100 Subject: [PATCH 06/30] feat(config): resolve stuck AWS direct RI-exchange rows ResolveRIExchangeDirectExecution locks the row, checks it against the operator expected status, the 10 minute settle age for submitted rows and the checked_at bounds on the database clock, and records the resolution beside the original status. A repeat of the same request replays. Refs #1836 Co-Authored-By: Claude Opus 5.5 --- internal/config/ri_exchange_direct_resolve.go | 57 +++++++ ...ore_postgres_ri_exchange_direct_resolve.go | 153 ++++++++++++++++++ ...xchange_direct_resolve_integration_test.go | 124 ++++++++++++++ 3 files changed, 334 insertions(+) create mode 100644 internal/config/ri_exchange_direct_resolve.go create mode 100644 internal/config/store_postgres_ri_exchange_direct_resolve.go create mode 100644 internal/config/store_postgres_ri_exchange_direct_resolve_integration_test.go diff --git a/internal/config/ri_exchange_direct_resolve.go b/internal/config/ri_exchange_direct_resolve.go new file mode 100644 index 000000000..7c03bfc93 --- /dev/null +++ b/internal/config/ri_exchange_direct_resolve.go @@ -0,0 +1,57 @@ +package config + +import ( + "context" + "time" +) + +// RIExchangeDirectResolveSubmittedAfter is how long a submitted AWS direct row +// must have existed before an operator may resolve it: the direct execute +// bounds its claim and provider call by one deadline taken before the row is +// saved (#1841), so no live process can still finalize it after this long. +// Measured on the database clock, like created_at. +const RIExchangeDirectResolveSubmittedAfter = 10 * time.Minute + +// RIExchangeDirectResolveNoteMax bounds resolution_note, in bytes. +const RIExchangeDirectResolveNoteMax = 1000 + +// RIExchangeDirectResolveUnknownCheckAfter is how long after an unknown row's +// completed_at the operator's AWS check must have been made, to allow for +// DescribeReservedInstances eventual consistency. +const RIExchangeDirectResolveUnknownCheckAfter = time.Minute + +// RIExchangeDirectResolve asks to settle a stuck AWS direct row on an +// operator's word. Outcome is completed or failed; ExpectedStatus is the +// status (submitted or unknown) the operator saw. Note is the complete text +// stored in resolution_note, built by the caller so a repeat compares equal +// as a whole. CheckedAt is when the operator checked AWS. +type RIExchangeDirectResolve struct { + ID string + Outcome string + ExpectedStatus string + Note string + CheckedAt time.Time + ResolvedByUserID *string + ResolvedBy string +} + +// RIExchangeDirectResolveResult is the row after the call. Status is the +// original status, unchanged. Replayed is true when an identical earlier +// request had already resolved the row and nothing was written. +type RIExchangeDirectResolveResult struct { + ID string + Status string + ResolvedStatus string + ResolvedAt time.Time + Note string + Replayed bool +} + +// RIExchangeDirectResolveStore is the store surface of the direct-row list and +// resolve endpoints. +type RIExchangeDirectResolveStore interface { + RIExchangeDirectListStore + ResolveRIExchangeDirectExecution(ctx context.Context, req RIExchangeDirectResolve) (*RIExchangeDirectResolveResult, error) +} + +var _ RIExchangeDirectResolveStore = (*PostgresStore)(nil) diff --git a/internal/config/store_postgres_ri_exchange_direct_resolve.go b/internal/config/store_postgres_ri_exchange_direct_resolve.go new file mode 100644 index 000000000..07e90eff5 --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_resolve.go @@ -0,0 +1,153 @@ +package config + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/jackc/pgx/v5" +) + +// riExchangeDirectResolveRow is the locked row the resolve rules run against. +// dbNow is the database clock after the lock was granted. +type riExchangeDirectResolveRow struct { + status string + createdAt time.Time + completedAt, resolvedAt *time.Time + resolvedStatus, note *string + dbNow time.Time +} + +// ResolveRIExchangeDirectExecution settles an AWS direct row an operator has +// verified at AWS. It records the resolution beside the row's original status +// and completed_at, which are never rewritten, and the in-flight guard then +// stops treating the row as holding its source RIs. One transaction locks the +// row, so two resolves and a concurrent finalize each see the other's commit. +// Every time comparison uses the database clock. +func (s *PostgresStore) ResolveRIExchangeDirectExecution(ctx context.Context, req RIExchangeDirectResolve) (*RIExchangeDirectResolveResult, error) { + if err := validateRIExchangeDirectResolve(req); err != nil { + return nil, err + } + res := &RIExchangeDirectResolveResult{ID: req.ID, ResolvedStatus: req.Outcome, Note: req.Note} + err := s.WithTx(ctx, func(tx pgx.Tx) error { + row, err := lockRIExchangeDirectForResolve(ctx, tx, req.ID) + if err != nil { + return err + } + if row.resolvedAt != nil { + return replayRIExchangeDirectResolve(req, row, res) + } + if err := checkRIExchangeDirectResolve(req, row); err != nil { + return err + } + return applyRIExchangeDirectResolve(ctx, tx, req, res) + }) + if err != nil { + return nil, err + } + return res, nil +} + +func validateRIExchangeDirectResolve(req RIExchangeDirectResolve) error { + switch { + case req.Outcome != RIExchangeDirectStatusCompleted && req.Outcome != RIExchangeDirectStatusFailed: + return fmt.Errorf("%w: outcome must be %q or %q", ErrRIExchangeResolveInvalid, RIExchangeDirectStatusCompleted, RIExchangeDirectStatusFailed) + case req.ExpectedStatus != RIExchangeDirectStatusSubmitted && req.ExpectedStatus != RIExchangeDirectStatusUnknown: + return fmt.Errorf("%w: expected status must be %q or %q", ErrRIExchangeResolveInvalid, RIExchangeDirectStatusSubmitted, RIExchangeDirectStatusUnknown) + case req.Note == "" || len(req.Note) > RIExchangeDirectResolveNoteMax: + return fmt.Errorf("%w: note must be 1 to %d bytes", ErrRIExchangeResolveInvalid, RIExchangeDirectResolveNoteMax) + case req.CheckedAt.IsZero(): + return fmt.Errorf("%w: the cloud check must name a time", ErrRIExchangeResolveInvalid) + case req.ResolvedBy == "": + return fmt.Errorf("%w: the resolver must be named", ErrRIExchangeResolveInvalid) + } + return nil +} + +func lockRIExchangeDirectForResolve(ctx context.Context, tx pgx.Tx, id string) (riExchangeDirectResolveRow, error) { + var row riExchangeDirectResolveRow + err := tx.QueryRow(ctx, ` + SELECT status, created_at, completed_at, resolved_at, resolved_status, resolution_note, clock_timestamp() + FROM ri_exchange_direct_executions + WHERE id = $1 AND provider = 'aws' + FOR UPDATE`, id, + ).Scan(&row.status, &row.createdAt, &row.completedAt, &row.resolvedAt, &row.resolvedStatus, &row.note, &row.dbNow) + if errors.Is(err, pgx.ErrNoRows) { + return row, ErrRIExchangeResolveNotFound + } + if err != nil { + return row, fmt.Errorf("failed to lock ri exchange direct execution %s: %w", id, err) + } + return row, nil +} + +// replayRIExchangeDirectResolve answers a request that found the row already +// resolved: success only when it holds exactly what this request would have +// written, else a conflict. +func replayRIExchangeDirectResolve(req RIExchangeDirectResolve, row riExchangeDirectResolveRow, res *RIExchangeDirectResolveResult) error { + if row.resolvedStatus != nil && *row.resolvedStatus == req.Outcome && row.note != nil && *row.note == req.Note { + res.Status = row.status + res.ResolvedAt = *row.resolvedAt + res.Replayed = true + return nil + } + return fmt.Errorf("%w: the exchange was already resolved as %s", ErrRIExchangeResolveConflict, derefString(row.resolvedStatus)) +} + +func derefString(s *string) string { + if s == nil { + return "" + } + return *s +} + +func checkRIExchangeDirectResolve(req RIExchangeDirectResolve, row riExchangeDirectResolveRow) error { + switch { + case row.status != RIExchangeDirectStatusSubmitted && row.status != RIExchangeDirectStatusUnknown: + return fmt.Errorf("%w: the exchange is already %s", ErrRIExchangeResolveConflict, row.status) + case row.status != req.ExpectedStatus: + return fmt.Errorf("%w: the exchange is %s, not %s", ErrRIExchangeResolveConflict, row.status, req.ExpectedStatus) + } + var earliest time.Time + if row.status == RIExchangeDirectStatusSubmitted { + earliest = row.createdAt.Add(RIExchangeDirectResolveSubmittedAfter) + if row.dbNow.Before(earliest) { + return fmt.Errorf("%w: the exchange may still be in flight; wait until %s", + ErrRIExchangeResolveConflict, earliest.UTC().Format(time.RFC3339)) + } + } else { + if row.completedAt == nil { + return errors.New("failed to resolve ri exchange direct execution: unknown row has no completed_at") + } + earliest = row.completedAt.Add(RIExchangeDirectResolveUnknownCheckAfter) + } + // Postgres keeps microseconds, so compare the operator's time at that + // precision: a nanosecond tail must neither pass nor fail a bound. + checked := req.CheckedAt.Truncate(time.Microsecond) + switch { + case checked.After(row.dbNow): + return fmt.Errorf("%w: checked_at is in the future", ErrRIExchangeResolveInvalid) + case checked.Before(earliest): + return fmt.Errorf("%w: checked_at must be at or after %s", ErrRIExchangeResolveInvalid, earliest.UTC().Format(time.RFC3339Nano)) + } + return nil +} + +func applyRIExchangeDirectResolve(ctx context.Context, tx pgx.Tx, req RIExchangeDirectResolve, res *RIExchangeDirectResolveResult) error { + err := tx.QueryRow(ctx, ` + UPDATE ri_exchange_direct_executions + SET resolved_status = $2, resolved_by_user_id = $3::uuid, resolved_by = $4, + resolved_at = clock_timestamp(), resolution_note = $5 + WHERE id = $1 AND provider = 'aws' AND resolved_at IS NULL AND status = $6 + RETURNING status, resolved_at`, + req.ID, req.Outcome, req.ResolvedByUserID, req.ResolvedBy, req.Note, req.ExpectedStatus, + ).Scan(&res.Status, &res.ResolvedAt) + if errors.Is(err, pgx.ErrNoRows) { + return fmt.Errorf("%w: the exchange changed while resolving", ErrRIExchangeResolveConflict) + } + if err != nil { + return fmt.Errorf("failed to resolve ri exchange direct execution %s: %w", req.ID, err) + } + return nil +} diff --git a/internal/config/store_postgres_ri_exchange_direct_resolve_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_resolve_integration_test.go new file mode 100644 index 000000000..8f0c1f953 --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_resolve_integration_test.go @@ -0,0 +1,124 @@ +//go:build integration + +package config + +import ( + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Real-PostgreSQL coverage for the #1836 store resolve. Fixtures set times +// relative to the database clock; the store must compare on that clock. + +// directAged inserts an AWS row aged on the DB clock: created_at is now()-age +// and, for a non-submitted row, completed_at is now()-completedAgo. +func (e *riExchangeIntegrationEnv) directAged(t *testing.T, status, age, completedAgo string) string { + t.Helper() + id := e.directRow(t, "aws", status, "ri-g1836-"+status+"-"+age) + _, err := e.poolA.Exec(e.ctx, ` + UPDATE ri_exchange_direct_executions + SET created_at = now() - ($2::text)::interval, + completed_at = CASE WHEN status = 'submitted' THEN NULL ELSE now() - ($3::text)::interval END + WHERE id = $1`, id, age, completedAgo) + require.NoError(t, err) + return id +} + +type directTimes struct{ created, completed time.Time } + +func (e *riExchangeIntegrationEnv) directTimes(t *testing.T, id string) directTimes { + t.Helper() + var d directTimes + var completed *time.Time + require.NoError(t, e.poolA.QueryRow(e.ctx, + `SELECT created_at, completed_at FROM ri_exchange_direct_executions WHERE id = $1`, id).Scan(&d.created, &completed)) + if completed != nil { + d.completed = *completed + } + return d +} + +func directResolveReq(id, outcome, expected string, checkedAt time.Time) RIExchangeDirectResolve { + return RIExchangeDirectResolve{ID: id, Outcome: outcome, ExpectedStatus: expected, Note: "operator:" + outcome + ": checked", + CheckedAt: checkedAt, ResolvedBy: "ops@example.com"} +} + +func (e *riExchangeIntegrationEnv) resolve(t *testing.T, req RIExchangeDirectResolve) (*RIExchangeDirectResolveResult, error) { + t.Helper() + return e.storeA.ResolveRIExchangeDirectExecution(e.ctx, req) +} + +// [fail-first] A submitted row resolves only after the settle age, measured on +// the DB clock; the original status and completed_at stay, the resolution is +// recorded, and the guard stops blocking. +func TestPostgresStoreDB_ResolveRIExchangeDirectExecution_SubmittedAgeBoundary(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + young := env.directAged(t, "submitted", "9 minutes 59 seconds", "0") + old := env.directAged(t, "submitted", "10 minutes 1 second", "0") + + _, err := env.resolve(t, directResolveReq(young, "failed", "submitted", time.Now().UTC())) + require.ErrorIs(t, err, ErrRIExchangeResolveConflict) + assert.Contains(t, err.Error(), "wait until") + require.ErrorIs(t, env.reserve(t, testAccountID(836100000001), "ri-g1836-submitted-9 minutes 59 seconds"), ErrRIExchangeSourceInFlight) + + times := env.directTimes(t, old) + uid := "7d444840-9dc0-11d1-b245-5ffdce74fad2" + req := directResolveReq(old, "failed", "submitted", times.created.Add(RIExchangeDirectResolveSubmittedAfter)) + req.ResolvedByUserID = &uid + res, err := env.resolve(t, req) + require.NoError(t, err) + assert.Equal(t, "submitted", res.Status, "the original status is returned unchanged") + assert.Equal(t, "failed", res.ResolvedStatus) + assert.False(t, res.Replayed) + + var status, rs, by, note, userID string + var completedNull bool + require.NoError(t, env.poolA.QueryRow(env.ctx, ` + SELECT status, resolved_status, resolved_by, resolution_note, resolved_by_user_id::text, completed_at IS NULL + FROM ri_exchange_direct_executions WHERE id = $1`, old).Scan(&status, &rs, &by, ¬e, &userID, &completedNull)) + assert.Equal(t, []any{"submitted", "failed", "ops@example.com", req.Note, uid, true}, []any{status, rs, by, note, userID, completedNull}) + require.NoError(t, env.reserve(t, testAccountID(836100000002), "ri-g1836-submitted-10 minutes 1 second"), "resolved row releases its RI") +} + +// [fail-first] Rows that must not resolve: Azure and unknown ids are not found, +// finalized rows and a wrong expected status conflict, bad input is invalid. +func TestPostgresStoreDB_ResolveRIExchangeDirectExecution_Refusals(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + now := time.Now().UTC() + az := env.insertDirect(t, directSpec{provider: "azure"}) + _, err := env.resolve(t, directResolveReq(az, "failed", "unknown", now)) + require.ErrorIs(t, err, ErrRIExchangeResolveNotFound) + _, err = env.resolve(t, directResolveReq("00000000-0000-0000-0000-000000000000", "failed", "unknown", now)) + require.ErrorIs(t, err, ErrRIExchangeResolveNotFound) + + for _, status := range []string{"completed", "failed"} { + id := env.directAged(t, status, "2 hours", "1 hour") + _, err = env.resolve(t, directResolveReq(id, "failed", "unknown", now)) + require.ErrorIs(t, err, ErrRIExchangeResolveConflict, status) + assert.Contains(t, err.Error(), status) + } + + unk := env.directAged(t, "unknown", "2 hours", "1 hour") + _, err = env.resolve(t, directResolveReq(unk, "failed", "submitted", now)) + require.ErrorIs(t, err, ErrRIExchangeResolveConflict, "expected_status mismatch") + + bad := []RIExchangeDirectResolve{ + directResolveReq(unk, "unknown", "unknown", now), + directResolveReq(unk, "failed", "completed", now), + directResolveReq(unk, "failed", "unknown", time.Time{}), + } + noNote := directResolveReq(unk, "failed", "unknown", now) + noNote.Note = "" + longNote := directResolveReq(unk, "failed", "unknown", now) + longNote.Note = strings.Repeat("x", RIExchangeDirectResolveNoteMax+1) + noBy := directResolveReq(unk, "failed", "unknown", now) + noBy.ResolvedBy = "" + for i, req := range append(bad, noNote, longNote, noBy) { + _, err = env.resolve(t, req) + require.ErrorIs(t, err, ErrRIExchangeResolveInvalid, "case %d", i) + } +} From da3220f7d6abcdf9eb50990891062abb7e00c122 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 8 Nov 2026 08:38:21 +0100 Subject: [PATCH 07/30] test(config): pin the resolve boundaries The checked_at bounds (not in the future; at or after created_at plus the settle age for submitted rows, and completed_at plus a minute for unknown rows) and the replay variants, on the database clock. Refs #1836 Co-Authored-By: Claude Opus 5.5 --- ...irect_resolve_boundary_integration_test.go | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 internal/config/store_postgres_ri_exchange_direct_resolve_boundary_integration_test.go diff --git a/internal/config/store_postgres_ri_exchange_direct_resolve_boundary_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_resolve_boundary_integration_test.go new file mode 100644 index 000000000..f9ffc28b6 --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_resolve_boundary_integration_test.go @@ -0,0 +1,84 @@ +//go:build integration + +package config + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Real-PostgreSQL boundary coverage for the #1836 resolve: the checked_at +// bounds and the replay variants. + +// [fail-first] checked_at bounds: exactly the bound is accepted and one +// microsecond less is refused, for both statuses and both outcomes; a time in +// the future and one in another zone are judged as instants. +func TestPostgresStoreDB_ResolveRIExchangeDirectExecution_CheckedAtBounds(t *testing.T) { + for _, outcome := range []string{"completed", "failed"} { + t.Run("submitted_"+outcome, func(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + id := env.directAged(t, "submitted", "1 hour", "0") + bound := env.directTimes(t, id).created.Add(RIExchangeDirectResolveSubmittedAfter) + + _, err := env.resolve(t, directResolveReq(id, outcome, "submitted", bound.Add(-time.Microsecond))) + require.ErrorIs(t, err, ErrRIExchangeResolveInvalid) + _, err = env.resolve(t, directResolveReq(id, outcome, "submitted", time.Now().UTC().Add(time.Hour))) + require.ErrorIs(t, err, ErrRIExchangeResolveInvalid, "future") + _, err = env.resolve(t, directResolveReq(id, outcome, "submitted", bound.Add(900*time.Nanosecond))) + require.NoError(t, err, "a nanosecond tail past the bound truncates to the bound and is accepted") + }) + t.Run("unknown_"+outcome, func(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + id := env.directAged(t, "unknown", "2 hours", "1 hour") + bound := env.directTimes(t, id).completed.Add(RIExchangeDirectResolveUnknownCheckAfter) + + _, err := env.resolve(t, directResolveReq(id, outcome, "unknown", bound.Add(-time.Microsecond))) + require.ErrorIs(t, err, ErrRIExchangeResolveInvalid) + zone := time.FixedZone("UTC+14", 14*3600) + _, err = env.resolve(t, directResolveReq(id, outcome, "unknown", bound.In(zone))) + require.NoError(t, err, "exactly the bound, given in another zone") + }) + } +} + +// [fail-first] An unknown row has no age rule, only the check bound: 59s after +// completion is refused, 61s accepted. +func TestPostgresStoreDB_ResolveRIExchangeDirectExecution_UnknownBound(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + a := env.directAged(t, "unknown", "2 minutes", "2 minutes") + completed := env.directTimes(t, a).completed + + _, err := env.resolve(t, directResolveReq(a, "failed", "unknown", completed.Add(59*time.Second))) + require.ErrorIs(t, err, ErrRIExchangeResolveInvalid) + _, err = env.resolve(t, directResolveReq(a, "failed", "unknown", completed.Add(61*time.Second))) + require.NoError(t, err) +} + +// [fail-first] A repeat of the same request replays and writes nothing; a +// different outcome or note conflicts. +func TestPostgresStoreDB_ResolveRIExchangeDirectExecution_Replay(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + id := env.directAged(t, "unknown", "2 hours", "1 hour") + req := directResolveReq(id, "failed", "unknown", time.Now().UTC().Add(-time.Minute)) + first, err := env.resolve(t, req) + require.NoError(t, err) + + again, err := env.resolve(t, req) + require.NoError(t, err) + assert.True(t, again.Replayed) + assert.Equal(t, "unknown", again.Status) + assert.True(t, first.ResolvedAt.Equal(again.ResolvedAt), "resolved_at is not rewritten") + + other := req + other.Outcome = "completed" + other.Note = "operator:completed: checked" + _, err = env.resolve(t, other) + require.ErrorIs(t, err, ErrRIExchangeResolveConflict) + diffNote := req + diffNote.Note = "a different note" + _, err = env.resolve(t, diffNote) + require.ErrorIs(t, err, ErrRIExchangeResolveConflict) +} From 09e7cde9e52b266eb31b2e1633107d6c55349256 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 8 Nov 2026 08:39:32 +0100 Subject: [PATCH 08/30] fix(config): log a provider outcome that contradicts an operator resolution Finalize returns resolved_status and logs at error level when a late provider outcome (completed or failed) disagrees with an operator's resolution; both facts stay on the row. Tests cover the race in both orders and concurrent resolves (one wins). Refs #1836 Co-Authored-By: Claude Opus 5.5 --- .../store_postgres_ri_exchange_direct.go | 16 +- ...ect_resolve_concurrent_integration_test.go | 155 ++++++++++++++++++ .../store_postgres_ri_exchange_direct_test.go | 8 +- 3 files changed, 171 insertions(+), 8 deletions(-) create mode 100644 internal/config/store_postgres_ri_exchange_direct_resolve_concurrent_integration_test.go diff --git a/internal/config/store_postgres_ri_exchange_direct.go b/internal/config/store_postgres_ri_exchange_direct.go index fc6637167..8c2df30bf 100644 --- a/internal/config/store_postgres_ri_exchange_direct.go +++ b/internal/config/store_postgres_ri_exchange_direct.go @@ -6,6 +6,7 @@ import ( "fmt" "strings" + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/jackc/pgx/v5" ) @@ -114,8 +115,10 @@ func (s *PostgresStore) FinalizeRIExchangeDirectExecution(ctx context.Context, i completed_at = now() WHERE id = $1 AND status = 'submitted' + RETURNING resolved_status ` - tag, err := s.db.Exec(ctx, query, + var resolved *string + err := s.db.QueryRow(ctx, query, id, outcome.Status, nullStringFromString(outcome.ProviderReference), @@ -123,12 +126,17 @@ func (s *PostgresStore) FinalizeRIExchangeDirectExecution(ctx context.Context, i nullStringFromString(outcome.NetPayableCurrency), nullStringFromString(outcome.ProviderStatus), nullStringFromString(outcome.Error), - ) + ).Scan(&resolved) + if errors.Is(err, pgx.ErrNoRows) { + return fmt.Errorf("finalize ri exchange direct execution %s: %w", id, ErrRIExchangeDirectExecutionNotSubmitted) + } if err != nil { return fmt.Errorf("failed to finalize ri exchange direct execution %s: %w", id, err) } - if tag.RowsAffected() == 0 { - return fmt.Errorf("finalize ri exchange direct execution %s: %w", id, ErrRIExchangeDirectExecutionNotSubmitted) + // A late finalize can outlive an operator's resolution. Both facts are + // kept; a contradiction is logged because no reader lists resolved rows. + if resolved != nil && outcome.Status != RIExchangeDirectStatusUnknown && *resolved != outcome.Status { + logging.Errorf("ri-exchange direct execute %s: provider outcome %q contradicts operator resolution %q", id, outcome.Status, *resolved) } return nil } diff --git a/internal/config/store_postgres_ri_exchange_direct_resolve_concurrent_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_resolve_concurrent_integration_test.go new file mode 100644 index 000000000..a19778a7c --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_resolve_concurrent_integration_test.go @@ -0,0 +1,155 @@ +//go:build integration + +package config + +import ( + "bytes" + "strings" + "sync" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Real-PostgreSQL coverage for #1836: the resolve against concurrent resolves +// and against a concurrent Finalize, in both orders. + +type lockedBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *lockedBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *lockedBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() +} + +// captureLogs routes the package logger to a locked buffer (Finalize runs in a +// goroutine). Not parallel: the logger is process-global. +func captureLogs(t *testing.T) *lockedBuffer { + t.Helper() + out := &lockedBuffer{} + prev := logging.SetOutput(out) + t.Cleanup(func() { logging.SetOutput(prev) }) + return out +} + +// [fail-first] Two concurrent resolves of one row: exactly one wins, and the +// stored resolution is the winner's. +func TestPostgresStoreDB_ResolveRIExchangeDirectExecution_ConcurrentOneWins(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + id := env.directAged(t, "unknown", "2 hours", "1 hour") + checked := time.Now().UTC().Add(-time.Minute) + reqs := []RIExchangeDirectResolve{directResolveReq(id, "failed", "unknown", checked), directResolveReq(id, "completed", "unknown", checked)} + stores := []*PostgresStore{env.storeA, env.storeB} + + errs := make([]error, 2) + start := make(chan struct{}) + var wg sync.WaitGroup + for i := range reqs { + wg.Add(1) + go func(i int) { + defer wg.Done() + <-start + _, errs[i] = stores[i].ResolveRIExchangeDirectExecution(env.ctx, reqs[i]) + }(i) + } + close(start) + wg.Wait() + + winner := -1 + for i, err := range errs { + if err == nil { + require.Equal(t, -1, winner, "two resolves both succeeded") + winner = i + continue + } + require.ErrorIs(t, err, ErrRIExchangeResolveConflict) + } + require.NotEqual(t, -1, winner, "one resolve must win") + var got string + require.NoError(t, env.poolA.QueryRow(env.ctx, `SELECT resolved_status FROM ri_exchange_direct_executions WHERE id = $1`, id).Scan(&got)) + assert.Equal(t, reqs[winner].Outcome, got) +} + +// holdRow opens a transaction that has written the row and not committed. +func (e *riExchangeIntegrationEnv) holdRow(t *testing.T, id, setSQL string) (commit func(), pid int) { + t.Helper() + tx, err := e.poolA.Begin(e.ctx) + require.NoError(t, err) + t.Cleanup(func() { _ = tx.Rollback(e.ctx) }) + tag, err := tx.Exec(e.ctx, `UPDATE ri_exchange_direct_executions SET `+setSQL+` WHERE id = $1`, id) + require.NoError(t, err) + require.EqualValues(t, 1, tag.RowsAffected()) + return func() { require.NoError(t, tx.Commit(e.ctx)) }, testhelpers.BackendPID(e.ctx, t, tx) +} + +// [fail-first] Finalize wins the row lock first: the resolve waits, then sees +// the new status and conflicts. +func TestPostgresStoreDB_ResolveRIExchangeDirectExecution_FinalizeFirst(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + id := env.directAged(t, "submitted", "1 hour", "0") + commit, pid := env.holdRow(t, id, `status = 'unknown', completed_at = now()`) + + done := make(chan error, 1) + go func() { + _, err := env.resolve(t, directResolveReq(id, "failed", "submitted", time.Now().UTC().Add(-time.Minute))) + done <- err + }() + if err, answered := testhelpers.AwaitBlockedBy(env.ctx, t, env.poolB, pid, done); answered { + t.Fatalf("resolve answered (%v) while a finalize held the row", err) + } + commit() + err := <-done + require.ErrorIs(t, err, ErrRIExchangeResolveConflict) + assert.Contains(t, err.Error(), "unknown") +} + +// [fail-first] Resolve wins first: Finalize waits on the row lock, then still +// stores the true outcome beside the resolution, and logs when they differ. +func TestPostgresStoreDB_ResolveRIExchangeDirectExecution_ResolveFirstThenFinalize(t *testing.T) { + for _, tc := range []struct { + name, resolved, finalStatus string + wantLog bool + }{ + {"contradiction is logged", "failed", RIExchangeDirectStatusCompleted, true}, + {"agreement is not", "completed", RIExchangeDirectStatusCompleted, false}, + {"unknown does not contradict", "failed", RIExchangeDirectStatusUnknown, false}, + } { + t.Run(tc.name, func(t *testing.T) { + logs := captureLogs(t) + env := newRIExchangeIntegrationEnv(t) + id := env.directAged(t, "submitted", "1 hour", "0") + commit, pid := env.holdRow(t, id, + `resolved_status = '`+tc.resolved+`', resolved_by = 'op', resolved_at = now(), resolution_note = 'n'`) + + done := make(chan error, 1) + go func() { + done <- env.storeB.FinalizeRIExchangeDirectExecution(env.ctx, id, RIExchangeDirectOutcome{Status: tc.finalStatus}) + }() + if err, answered := testhelpers.AwaitBlockedBy(env.ctx, t, env.poolA, pid, done); answered { + t.Fatalf("finalize answered (%v) while the resolution was uncommitted", err) + } + commit() + require.NoError(t, <-done) + + var status, rs string + require.NoError(t, env.poolA.QueryRow(env.ctx, + `SELECT status, resolved_status FROM ri_exchange_direct_executions WHERE id = $1`, id).Scan(&status, &rs)) + assert.Equal(t, tc.finalStatus, status, "the provider outcome is stored") + assert.Equal(t, tc.resolved, rs, "beside the operator's resolution") + assert.Equal(t, tc.wantLog, strings.Contains(logs.String(), "contradicts operator resolution"), logs.String()) + }) + } +} diff --git a/internal/config/store_postgres_ri_exchange_direct_test.go b/internal/config/store_postgres_ri_exchange_direct_test.go index a0513f2a7..c87a6e9b5 100644 --- a/internal/config/store_postgres_ri_exchange_direct_test.go +++ b/internal/config/store_postgres_ri_exchange_direct_test.go @@ -135,9 +135,9 @@ func TestPGXMock_FinalizeRIExchangeDirectExecution_CompareAndSet(t *testing.T) { t.Run("submitted row is finalized", func(t *testing.T) { mock := newMock(t) store := storeWith(mock) - mock.ExpectExec(`UPDATE ri_exchange_direct_executions(.|\n)*WHERE id = \$1(.|\n)*AND status = 'submitted'`). + mock.ExpectQuery(`UPDATE ri_exchange_direct_executions(.|\n)*WHERE id = \$1(.|\n)*AND status = 'submitted'(.|\n)*RETURNING resolved_status`). WithArgs(directExecTestID, "completed", pgxmock.AnyArg(), outcome.NetPayable, pgxmock.AnyArg(), pgxmock.AnyArg(), pgxmock.AnyArg()). - WillReturnResult(pgxmock.NewResult("UPDATE", 1)) + WillReturnRows(pgxmock.NewRows([]string{"resolved_status"}).AddRow((*string)(nil))) require.NoError(t, store.FinalizeRIExchangeDirectExecution(context.Background(), directExecTestID, outcome)) assert.NoError(t, mock.ExpectationsWereMet()) }) @@ -145,9 +145,9 @@ func TestPGXMock_FinalizeRIExchangeDirectExecution_CompareAndSet(t *testing.T) { t.Run("finalized or missing row is refused", func(t *testing.T) { mock := newMock(t) store := storeWith(mock) - mock.ExpectExec("UPDATE ri_exchange_direct_executions"). + mock.ExpectQuery("UPDATE ri_exchange_direct_executions"). WithArgs(anyArgsCfg(7)...). - WillReturnResult(pgxmock.NewResult("UPDATE", 0)) + WillReturnRows(pgxmock.NewRows([]string{"resolved_status"})) err := store.FinalizeRIExchangeDirectExecution(context.Background(), directExecTestID, outcome) require.ErrorIs(t, err, ErrRIExchangeDirectExecutionNotSubmitted) assert.NoError(t, mock.ExpectationsWereMet()) From 96c013284043b229860bc5f9cdaa81bf06461af1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 8 Nov 2026 09:14:06 +0100 Subject: [PATCH 09/30] refactor(api): share the RI-exchange constraint check Split the account, region and amount inputs of the history resolve constraint check into requireRIExchangeConstraintsFor so the direct-row resolve can reuse it. Behaviour of the history resolve is unchanged. Refs #1836 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_ri_exchange_resolve.go | 12 +++++- .../api/handler_ri_exchange_resolve_test.go | 39 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/internal/api/handler_ri_exchange_resolve.go b/internal/api/handler_ri_exchange_resolve.go index be5924492..da270fde5 100644 --- a/internal/api/handler_ri_exchange_resolve.go +++ b/internal/api/handler_ri_exchange_resolve.go @@ -118,6 +118,16 @@ func (h *Handler) requireRIExchangeResolveConstraints(ctx context.Context, sessi if err != nil { return fmt.Errorf("failed to resolve cloud account scope: %w", err) } + return h.requireRIExchangeConstraintsFor(ctx, session, cloudAccountID, region, func() (float64, error) { + return h.riExchangeHoldAmount(ctx, id) + }) +} + +// requireRIExchangeConstraintsFor is the constraint check shared by the +// history resolve and the direct-row resolve: the grant must allow the AWS EC2 +// exchange in the given account and region, and when it caps the amount, +// amount() must fit under the cap. A grant without a cap never calls amount. +func (h *Handler) requireRIExchangeConstraintsFor(ctx context.Context, session *Session, cloudAccountID, region string, amount func() (float64, error)) error { c := auth.PermissionConstraints{ AccountIDs: []string{cloudAccountID}, Providers: []string{string(common.ProviderAWS)}, @@ -135,7 +145,7 @@ func (h *Handler) requireRIExchangeResolveConstraints(ctx context.Context, sessi if otherErr := check(0); otherErr != nil { return otherErr } - due, err := h.riExchangeHoldAmount(ctx, id) + due, err := amount() if err != nil { return err } diff --git a/internal/api/handler_ri_exchange_resolve_test.go b/internal/api/handler_ri_exchange_resolve_test.go index 17fd9b6aa..4e5384fd9 100644 --- a/internal/api/handler_ri_exchange_resolve_test.go +++ b/internal/api/handler_ri_exchange_resolve_test.go @@ -288,3 +288,42 @@ func TestResolveRIExchange_CappedGrantMissingHoldIsNotFound(t *testing.T) { require.ErrorIs(t, err, errNotFound) assert.Zero(t, store.called) } + +// The constraints check must use the checked region and the deployment's own +// account, not a fixed pair: each case differs from the other cases in exactly +// the dimension a hard-coded region or a swapped account would hide. +func TestResolveRIExchange_ConstraintsUseTheCheckedRegionAndTheDeploymentAccount(t *testing.T) { + grant := func(c auth.PermissionConstraints) []auth.Permission { + return []auth.Permission{{Action: auth.ActionExecute, Resource: "ri-exchange", Constraints: &c}} + } + tests := []struct { + name string + perms []auth.Permission + region string + deployment string + want int // 0 = allowed + }{ + {"grant for the checked region, which is not us-east-1", grant(auth.PermissionConstraints{Regions: []string{"eu-west-1"}}), "eu-west-1", scopedInAccount, 0}, + {"grant for us-east-1 against another checked region", grant(auth.PermissionConstraints{Regions: []string{"us-east-1"}}), "eu-west-1", scopedInAccount, 403}, + {"grant for the deployment's account", grant(auth.PermissionConstraints{AccountIDs: []string{scopedInAccount}}), "us-east-1", scopedInAccount, 0}, + {"grant for another account than the deployment's", grant(auth.PermissionConstraints{AccountIDs: []string{scopedInAccount}}), "us-east-1", scopedOutAccount, 403}, + {"grant for the other account, matching the deployment", grant(auth.PermissionConstraints{AccountIDs: []string{scopedOutAccount}}), "us-east-1", scopedOutAccount, 0}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + store := &resolveRIStore{MockConfigStore: new(MockConfigStore)} + h, req := apgCaller{perms: tc.perms}.handlerAndReq(store.MockConfigStore, + strings.ReplaceAll(riResolveFailedBody, "us-east-1", tc.region)) + h.config = store + h.reshapeAccountResolver = func(context.Context) (string, error) { return tc.deployment, nil } + _, err := h.resolveRIExchange(context.Background(), req, resolveRIID) + if tc.want == 0 { + require.NoError(t, err) + assert.Equal(t, 1, store.called) + return + } + assert.Equal(t, tc.want, clientCode(t, err)) + assert.Zero(t, store.called, "a refused caller must not reach the store") + }) + } +} From e08eb031e0dca49e31c090a0b374ede230026c65 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 8 Nov 2026 09:19:30 +0100 Subject: [PATCH 10/30] feat(api): shape the direct-exchange list items and query The list item is built in one place from a stored row, so the raw error, the executing user id and the API key id are never exposed; error_summary goes through SanitizeStoredError. Adds query validation, the opaque keyset cursor and the resolvable hint. Refs #1836 Co-Authored-By: Claude Opus 5.5 --- ...ndler_ri_exchange_direct_list_item_test.go | 139 ++++++++++++++++++ .../api/handler_ri_exchange_direct_resolve.go | 134 +++++++++++++++++ 2 files changed, 273 insertions(+) create mode 100644 internal/api/handler_ri_exchange_direct_list_item_test.go create mode 100644 internal/api/handler_ri_exchange_direct_resolve.go diff --git a/internal/api/handler_ri_exchange_direct_list_item_test.go b/internal/api/handler_ri_exchange_direct_list_item_test.go new file mode 100644 index 000000000..e9cc6896a --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_list_item_test.go @@ -0,0 +1,139 @@ +package api + +import ( + "encoding/json" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" +) + +// Unit tests for the pieces of GET /api/ri-exchange/direct-executions that need +// no store (#1836): what a stored row exposes, the resolvable hint, query +// validation and the cursor. + +const ( + dlAcctA = scopedInAccount + dlAcctB = scopedOutAccount + dlID1 = "00000000-0000-4000-8000-000000000001" + dlID2 = "00000000-0000-4000-8000-000000000002" + dlID3 = "00000000-0000-4000-8000-000000000003" + dlID4 = "00000000-0000-4000-8000-000000000004" +) + +var dlBase = time.Now().UTC().Truncate(time.Microsecond).Add(-time.Hour) + +func dlPtr(s string) *string { return &s } + +func dlRow(id string, account *string, region, status string, minutes int) config.RIExchangeDirectRow { + at := dlBase.Add(time.Duration(minutes) * time.Minute) + r := config.RIExchangeDirectRow{ + ID: id, Status: status, CloudAccountID: account, Region: region, + SourceIDs: []string{"ri-" + id}, Targets: json.RawMessage(`[{"offering_id":"o","count":1}]`), + MaxPaymentDue: "100", MaxPaymentCurrency: "USD", ExecutedBy: "alice@example.com", CreatedAt: at, + } + if status != "submitted" { + r.CompletedAt = &at + } + return r +} + +func keysOf[V any](m map[string]V) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + return keys +} + +// [fail-first] The response carries exactly the allowlisted keys: never the raw +// error, the executing user id or the API key id. +func TestRIExchangeDirectListItem_KeysAreAllowlisted(t *testing.T) { + row := dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1) + row.Error = "InvalidParameterValue for account 123456789012 from 10.1.2.3: raw provider text" + + raw, err := json.Marshal(riExchangeDirectListResponse{ + Executions: []riExchangeDirectListItem{newRIExchangeDirectListItem(&row, time.Now())}, NextCursor: ""}) + require.NoError(t, err) + + var top map[string]json.RawMessage + require.NoError(t, json.Unmarshal(raw, &top)) + assert.ElementsMatch(t, []string{"executions", "next_cursor"}, keysOf(top)) + var items []map[string]any + require.NoError(t, json.Unmarshal(top["executions"], &items)) + require.Len(t, items, 1) + assert.ElementsMatch(t, []string{"id", "status", "provider", "region", "source_ids", "targets", "max_payment_due", + "max_payment_currency", "created_at", "executed_by", "error_summary", "resolvable", "resolvable_at"}, keysOf(items[0])) + + assert.Equal(t, purchase.SanitizeStoredError(row.Error), items[0]["error_summary"]) + for _, leak := range []string{"123456789012", "10.1.2.3", "raw provider text"} { + assert.NotContains(t, string(raw), leak) + } + assert.Equal(t, "aws", items[0]["provider"]) +} + +func TestRIExchangeDirectListItem_ResolvableHint(t *testing.T) { + now := time.Now().UTC() + at := func(r config.RIExchangeDirectRow, created time.Time) config.RIExchangeDirectRow { + r.CreatedAt = created + if r.CompletedAt != nil { + r.CompletedAt = &created + } + return r + } + sub := at(dlRow(dlID1, nil, "eu-west-1", "submitted", 0), now.Add(-5*time.Minute)) + oldSub := at(dlRow(dlID2, nil, "eu-west-1", "submitted", 0), now.Add(-config.RIExchangeDirectResolveSubmittedAfter-time.Minute)) + unk := at(dlRow(dlID3, nil, "eu-west-1", "unknown", 0), now.Add(-2*time.Minute)) + freshUnk := at(dlRow(dlID4, nil, "eu-west-1", "unknown", 0), now.Add(-10*time.Second)) + + got := func(r config.RIExchangeDirectRow) riExchangeDirectListItem { + return newRIExchangeDirectListItem(&r, now) + } + assert.False(t, got(sub).Resolvable) + assert.True(t, got(sub).ResolvableAt.Equal(sub.CreatedAt.Add(config.RIExchangeDirectResolveSubmittedAfter))) + assert.True(t, got(oldSub).Resolvable) + assert.True(t, got(unk).Resolvable) + assert.True(t, got(unk).ResolvableAt.Equal(unk.CompletedAt.Add(config.RIExchangeDirectResolveUnknownCheckAfter))) + assert.False(t, got(freshUnk).Resolvable) +} + +func TestParseRIExchangeDirectListQuery(t *testing.T) { + for name, q := range map[string]map[string]string{ + "bad status": {"status": "completed"}, + "limit zero": {"limit": "0"}, + "limit too big": {"limit": "201"}, + "limit not int": {"limit": "ten"}, + "bad region": {"region": "EU West"}, + "bad cursor": {"cursor": "!!!"}, + "cursor no id": {"cursor": "MjAyNi0wMS0wMVQwMDowMDowMFp8bm90LWEtdXVpZA"}, + "ri_id control": {"ri_id": "ri\n1"}, + "ri_id too long": {"ri_id": strings.Repeat("a", riExchangeDirectRIIDMax+1)}, + } { + _, err := parseRIExchangeDirectListQuery(q) + assert.Equal(t, 400, clientCode(t, err), name) + } + + f, err := parseRIExchangeDirectListQuery(map[string]string{"status": "unknown", "region": "eu-west-1", "ri_id": "RI-X", "limit": "7"}) + require.NoError(t, err) + assert.Equal(t, []string{"unknown"}, f.Statuses) + assert.Equal(t, "eu-west-1", f.Region) + assert.Equal(t, "RI-X", f.RIID) + assert.Equal(t, 7, f.Limit) + + f, err = parseRIExchangeDirectListQuery(nil) + require.NoError(t, err) + assert.Equal(t, config.RIExchangeDirectListDefaultLimit, f.Limit) +} + +func TestRIExchangeDirectCursor_RoundTripKeepsMicroseconds(t *testing.T) { + at := time.Date(2026, 11, 7, 10, 20, 30, 123456000, time.UTC) + got, err := decodeRIExchangeDirectCursor(encodeRIExchangeDirectCursor(&config.RIExchangeDirectCursor{CreatedAt: at, ID: dlID2})) + require.NoError(t, err) + assert.True(t, got.CreatedAt.Equal(at)) + assert.Equal(t, dlID2, got.ID) +} diff --git a/internal/api/handler_ri_exchange_direct_resolve.go b/internal/api/handler_ri_exchange_direct_resolve.go new file mode 100644 index 000000000..df5288b22 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_resolve.go @@ -0,0 +1,134 @@ +package api + +import ( + "encoding/base64" + "encoding/json" + "fmt" + "strconv" + "strings" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" +) + +// Operator list of AWS direct RI-exchange rows that still hold their source +// RIs (#1836). An accept whose outcome was lost leaves the row submitted or +// unknown, and the in-flight guard then refuses every later exchange of those +// RIs until a person who checked AWS resolves it. + +const riExchangeDirectRIIDMax = 200 + +type riExchangeDirectListItem struct { + ID string `json:"id"` + Status string `json:"status"` + Provider string `json:"provider"` + Region string `json:"region"` + SourceIDs []string `json:"source_ids"` + Targets json.RawMessage `json:"targets"` + MaxPaymentDue string `json:"max_payment_due"` + MaxPaymentCurrency string `json:"max_payment_currency"` + CreatedAt time.Time `json:"created_at"` + ExecutedBy string `json:"executed_by"` + ErrorSummary string `json:"error_summary"` + Resolvable bool `json:"resolvable"` + ResolvableAt time.Time `json:"resolvable_at"` +} + +type riExchangeDirectListResponse struct { + Executions []riExchangeDirectListItem `json:"executions"` + NextCursor string `json:"next_cursor"` +} + +func parseRIExchangeDirectListQuery(q map[string]string) (config.RIExchangeDirectListFilter, error) { + var f config.RIExchangeDirectListFilter + if s := q["status"]; s != "" { + if s != config.RIExchangeDirectStatusSubmitted && s != config.RIExchangeDirectStatusUnknown { + return f, NewClientError(400, `status must be "submitted" or "unknown"`) + } + f.Statuses = []string{s} + } + if r := q["region"]; r != "" { + if !riExchangeResolveRegionRe.MatchString(r) { + return f, NewClientError(400, "region is not a valid AWS region") + } + f.Region = r + } + if id := q["ri_id"]; id != "" { + if len(id) > riExchangeDirectRIIDMax || hasControlRune(id) { + return f, NewClientError(400, "ri_id is not a valid reserved instance id") + } + f.RIID = id + } + return parseRIExchangeDirectListPaging(q, f) +} + +// parseRIExchangeDirectListPaging reads the limit and cursor into f. +func parseRIExchangeDirectListPaging(q map[string]string, f config.RIExchangeDirectListFilter) (config.RIExchangeDirectListFilter, error) { + f.Limit = config.RIExchangeDirectListDefaultLimit + if l := q["limit"]; l != "" { + n, err := strconv.Atoi(l) + if err != nil || n < 1 || n > config.RIExchangeDirectListMaxLimit { + return f, NewClientError(400, fmt.Sprintf("limit must be 1 to %d", config.RIExchangeDirectListMaxLimit)) + } + f.Limit = n + } + if c := q["cursor"]; c != "" { + cur, err := decodeRIExchangeDirectCursor(c) + if err != nil { + return f, err + } + f.After = cur + } + return f, nil +} + +// The cursor is opaque to callers: the created_at and id of the last row of +// the previous page. It is a position, not a credential, because the store +// only ever returns rows the caller may see. +func encodeRIExchangeDirectCursor(c *config.RIExchangeDirectCursor) string { + return base64.RawURLEncoding.EncodeToString([]byte(c.CreatedAt.UTC().Format(time.RFC3339Nano) + "|" + c.ID)) +} + +func decodeRIExchangeDirectCursor(s string) (*config.RIExchangeDirectCursor, error) { + raw, err := base64.RawURLEncoding.DecodeString(s) + bad := NewClientError(400, "cursor is not valid") + if err != nil { + return nil, bad + } + at, id, found := strings.Cut(string(raw), "|") + if !found || validateUUID(id) != nil { + return nil, bad + } + t, err := time.Parse(time.RFC3339Nano, at) + if err != nil { + return nil, bad + } + return &config.RIExchangeDirectCursor{CreatedAt: t, ID: id}, nil +} + +// newRIExchangeDirectListItem is the one place a stored row becomes a response +// item, so the exposed fields are exactly these: the raw stored error, the +// executing user id and the API key id never leave the server. +func newRIExchangeDirectListItem(r *config.RIExchangeDirectRow, now time.Time) riExchangeDirectListItem { + at := resolvableRIExchangeDirectAt(r) + return riExchangeDirectListItem{ + ID: r.ID, Status: r.Status, Provider: string(common.ProviderAWS), Region: r.Region, + SourceIDs: r.SourceIDs, Targets: r.Targets, + MaxPaymentDue: r.MaxPaymentDue, MaxPaymentCurrency: r.MaxPaymentCurrency, + CreatedAt: r.CreatedAt.UTC(), ExecutedBy: r.ExecutedBy, + ErrorSummary: purchase.SanitizeStoredError(r.Error), + Resolvable: !now.Before(at), ResolvableAt: at, + } +} + +// resolvableRIExchangeDirectAt is when the store's rules first admit a +// resolve: a submitted row once its settle age has passed, an unknown row once +// the operator's AWS check can postdate its completion by the required margin. +func resolvableRIExchangeDirectAt(r *config.RIExchangeDirectRow) time.Time { + if r.Status == config.RIExchangeDirectStatusSubmitted || r.CompletedAt == nil { + return r.CreatedAt.Add(config.RIExchangeDirectResolveSubmittedAfter).UTC() + } + return r.CompletedAt.Add(config.RIExchangeDirectResolveUnknownCheckAfter).UTC() +} From a4bc7443a2d9f578110c3d49c10376020b558f37 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 8 Nov 2026 09:24:22 +0100 Subject: [PATCH 11/30] feat(api): list unresolved AWS direct exchanges GET handler for rows that still hold their source RIs. Requires execute:ri-exchange. The (account, region) pairs of unresolved rows are kept only when the session scope and the grant, evaluated by the authorization service as the direct execute does, cover them; the store then pages only those pairs, so no cursor can point at a hidden row. Refs #1836 Co-Authored-By: Claude Opus 5.5 --- .../api/handler_ri_exchange_direct_list.go | 105 ++++++++ .../handler_ri_exchange_direct_list_test.go | 250 ++++++++++++++++++ 2 files changed, 355 insertions(+) create mode 100644 internal/api/handler_ri_exchange_direct_list.go create mode 100644 internal/api/handler_ri_exchange_direct_list_test.go diff --git a/internal/api/handler_ri_exchange_direct_list.go b/internal/api/handler_ri_exchange_direct_list.go new file mode 100644 index 000000000..be016c514 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_list.go @@ -0,0 +1,105 @@ +package api + +import ( + "context" + "fmt" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" +) + +func (h *Handler) listRIExchangeDirectExecutions(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { + session, err := h.requirePermission(ctx, req, "execute", "ri-exchange") + if err != nil { + return nil, err + } + filter, err := parseRIExchangeDirectListQuery(req.QueryStringParameters) + if err != nil { + return nil, err + } + store, ok := h.config.(config.RIExchangeDirectListStore) + if !ok { + return nil, NewClientError(503, "RI exchange resolve storage is unavailable") + } + allowed, err := h.visibleRIExchangeDirectPairs(ctx, session, store) + if err != nil { + return nil, err + } + filter.Allowed = allowed + page, err := store.ListUnresolvedRIExchangeDirectExecutions(ctx, filter) + if err := mapRIExchangeResolveError(err); err != nil { + return nil, err + } + now := time.Now().UTC() + resp := riExchangeDirectListResponse{Executions: make([]riExchangeDirectListItem, 0, len(page.Rows))} + for i := range page.Rows { + resp.Executions = append(resp.Executions, newRIExchangeDirectListItem(&page.Rows[i], now)) + } + if page.Next != nil { + resp.NextCursor = encodeRIExchangeDirectCursor(page.Next) + } + return resp, nil +} + +// visibleRIExchangeDirectPairs keeps the (account, region) pairs of unresolved +// rows that the session may see: its account scope allows the account (a row +// with no registered account needs an unrestricted scope) and the grant on +// execute:ri-exchange covers the same account, provider, service and region +// the direct execute checks. The grant is evaluated by the authorization +// service, never reimplemented here, so OR'd grants, a user API key's own +// narrower permissions and region spelling behave as they do on execute. +func (h *Handler) visibleRIExchangeDirectPairs(ctx context.Context, session *Session, store config.RIExchangeDirectListStore) ([]config.RIExchangeDirectAccountRegion, error) { + scope, err := h.getAccountScope(ctx, session) + if err != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", err) + } + pairs, err := store.ListUnresolvedRIExchangeDirectAccountRegions(ctx) + if err != nil { + return nil, fmt.Errorf("failed to list unresolved direct exchange accounts: %w", err) + } + var visible []config.RIExchangeDirectAccountRegion + for _, p := range pairs { + account, inScope := riExchangeDirectPairAccount(scope, p) + if !inScope { + continue + } + granted, err := h.riExchangeDirectPairGranted(ctx, session, account, p.Region) + if err != nil { + return nil, err + } + if granted { + visible = append(visible, p) + } + } + return visible, nil +} + +// riExchangeDirectPairAccount returns the account the grant is checked +// against (the unattributed sentinel for a row with no registered account) and +// whether the session's account scope covers it. A row with no account needs +// an unrestricted scope. +func riExchangeDirectPairAccount(scope auth.AccountScope, p config.RIExchangeDirectAccountRegion) (string, bool) { + if p.CloudAccountID == nil { + return unattributedAccountConstraint, scope.AllowsAll() + } + return *p.CloudAccountID, scope.Allows(*p.CloudAccountID, "") +} + +// riExchangeDirectPairGranted asks the authorization service whether the +// grant covers an AWS EC2 exchange in the account and region. A denial is +// false; any other failure is an error. +func (h *Handler) riExchangeDirectPairGranted(ctx context.Context, session *Session, account, region string) (bool, error) { + err := h.requirePermissionConstraints(ctx, session, "execute", "ri-exchange", []auth.PermissionConstraints{{ + AccountIDs: []string{account}, + Providers: []string{string(common.ProviderAWS)}, + Services: []string{string(common.ServiceEC2)}, + Regions: []string{region}, + }}) + if ce, isClient := IsClientError(err); isClient && ce.code == 403 { + return false, nil + } + return err == nil, err +} diff --git a/internal/api/handler_ri_exchange_direct_list_test.go b/internal/api/handler_ri_exchange_direct_list_test.go new file mode 100644 index 000000000..b5b2459ca --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_list_test.go @@ -0,0 +1,250 @@ +package api + +import ( + "context" + "encoding/json" + "sort" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Handler tests for GET /api/ri-exchange/direct-executions (#1836). The store +// is an in-memory fixture that applies the allowed set and the keyset the way +// the SQL does (covered against real PostgreSQL in internal/config); these +// tests pin which (account, region) pairs the handler hands it. + +type directListStore struct { + *MockConfigStore + rows []config.RIExchangeDirectRow + lastFilter config.RIExchangeDirectListFilter + listCalls int + pairCalls int +} + +func (s *directListStore) ListUnresolvedRIExchangeDirectAccountRegions(context.Context) ([]config.RIExchangeDirectAccountRegion, error) { + s.pairCalls++ + seen := map[string]bool{} + var out []config.RIExchangeDirectAccountRegion + for _, r := range s.rows { + k := pairKey(r.CloudAccountID, r.Region) + if !seen[k] { + seen[k] = true + out = append(out, config.RIExchangeDirectAccountRegion{CloudAccountID: r.CloudAccountID, Region: r.Region}) + } + } + return out, nil +} + +func pairKey(account *string, region string) string { + if account == nil { + return "|" + region + } + return *account + "|" + region +} + +func (s *directListStore) ListUnresolvedRIExchangeDirectExecutions(_ context.Context, f config.RIExchangeDirectListFilter) (*config.RIExchangeDirectListPage, error) { + s.listCalls++ + s.lastFilter = f + allowed := map[string]bool{} + for _, p := range f.Allowed { + allowed[pairKey(p.CloudAccountID, p.Region)] = true + } + var rows []config.RIExchangeDirectRow + for _, r := range s.rows { + if !allowed[pairKey(r.CloudAccountID, r.Region)] { + continue + } + if f.After != nil && !(r.CreatedAt.After(f.After.CreatedAt) || r.CreatedAt.Equal(f.After.CreatedAt) && r.ID > f.After.ID) { + continue + } + rows = append(rows, r) + } + sort.Slice(rows, func(i, j int) bool { + if !rows[i].CreatedAt.Equal(rows[j].CreatedAt) { + return rows[i].CreatedAt.Before(rows[j].CreatedAt) + } + return rows[i].ID < rows[j].ID + }) + page := &config.RIExchangeDirectListPage{} + if f.Limit > 0 && len(rows) > f.Limit { + rows = rows[:f.Limit] + page.Next = &config.RIExchangeDirectCursor{CreatedAt: rows[f.Limit-1].CreatedAt, ID: rows[f.Limit-1].ID} + } + page.Rows = rows + return page, nil +} + +func (s *directListStore) GetRIExchangeDirectExecution(context.Context, string) (*config.RIExchangeDirectRow, error) { + return nil, config.ErrRIExchangeResolveNotFound +} + +func dlStore(rows ...config.RIExchangeDirectRow) *directListStore { + s := &directListStore{MockConfigStore: new(MockConfigStore), rows: rows} + s.ListCloudAccountsFn = func(context.Context, config.CloudAccountFilter) ([]config.CloudAccount, error) { return nil, nil } + return s +} + +// dlCall runs the handler as a session user holding perms, scoped to accounts +// (nil means unrestricted). +func dlCall(t *testing.T, store *directListStore, perms []auth.Permission, accounts []string, query map[string]string) (any, error) { + t.Helper() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "tok").Return(&Session{UserID: apgUserID}, nil).Maybe() + mockAuth.grantPermissionsScoped(perms, accounts) + h := &Handler{config: store, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer tok"}, QueryStringParameters: query} + return h.listRIExchangeDirectExecutions(context.Background(), req) +} + +func dlIDs(t *testing.T, out any) []string { + t.Helper() + resp, ok := out.(riExchangeDirectListResponse) + require.True(t, ok) + ids := make([]string, len(resp.Executions)) + for i, e := range resp.Executions { + ids[i] = e.ID + } + return ids +} + +func regionGrant(regions ...string) []auth.Permission { + var perms []auth.Permission + for _, r := range regions { + perms = append(perms, auth.Permission{Action: auth.ActionExecute, Resource: "ri-exchange", + Constraints: &auth.PermissionConstraints{Regions: []string{r}}}) + } + return perms +} + +func TestListRIExchangeDirectExecutions_NeedsExecutePermission(t *testing.T) { + store := dlStore(dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1)) + _, err := dlCall(t, store, []auth.Permission{{Action: auth.ActionView, Resource: auth.ResourcePurchases}}, nil, nil) + assert.Equal(t, 403, clientCode(t, err)) + assert.Zero(t, store.pairCalls+store.listCalls, "a refused caller must not reach the store") +} + +func TestListRIExchangeDirectExecutions_AccountScope(t *testing.T) { + store := dlStore( + dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1), + dlRow(dlID2, dlPtr(dlAcctB), "eu-west-1", "unknown", 2), + dlRow(dlID3, nil, "eu-west-1", "submitted", 3), + ) + t.Run("scoped session sees its account only, never an unattributed row", func(t *testing.T) { + out, err := dlCall(t, store, riExecute, []string{dlAcctA}, nil) + require.NoError(t, err) + assert.Equal(t, []string{dlID1}, dlIDs(t, out)) + }) + t.Run("unrestricted session sees every account and the unattributed row", func(t *testing.T) { + out, err := dlCall(t, store, riExecute, nil, nil) + require.NoError(t, err) + assert.Equal(t, []string{dlID1, dlID2, dlID3}, dlIDs(t, out)) + }) + t.Run("nothing visible is an empty list, not an error", func(t *testing.T) { + out, err := dlCall(t, store, riExecute, []string{"33333333-3333-4333-8333-333333333333"}, nil) + require.NoError(t, err) + raw, _ := json.Marshal(out) + assert.JSONEq(t, `{"executions":[],"next_cursor":""}`, string(raw)) + }) +} + +func TestListRIExchangeDirectExecutions_GrantRegions(t *testing.T) { + store := dlStore( + dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1), + dlRow(dlID2, dlPtr(dlAcctA), "us-east-1", "unknown", 2), + dlRow(dlID3, dlPtr(dlAcctA), "ap-south-1", "unknown", 3), + ) + t.Run("a grant restricted to one region", func(t *testing.T) { + out, err := dlCall(t, store, regionGrant("us-east-1"), nil, nil) + require.NoError(t, err) + assert.Equal(t, []string{dlID2}, dlIDs(t, out)) + }) + t.Run("two grants are OR'd", func(t *testing.T) { + out, err := dlCall(t, store, regionGrant("us-east-1", "eu-west-1"), nil, nil) + require.NoError(t, err) + assert.Equal(t, []string{dlID1, dlID2}, dlIDs(t, out)) + }) + t.Run("region spelling is not case sensitive", func(t *testing.T) { + out, err := dlCall(t, store, regionGrant("EU-WEST-1"), nil, nil) + require.NoError(t, err) + assert.Equal(t, []string{dlID1}, dlIDs(t, out)) + }) + t.Run("an account-constrained grant hides other accounts' rows", func(t *testing.T) { + other := dlStore(dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1), dlRow(dlID2, dlPtr(dlAcctB), "eu-west-1", "unknown", 2)) + perms := []auth.Permission{{Action: auth.ActionExecute, Resource: "ri-exchange", + Constraints: &auth.PermissionConstraints{AccountIDs: []string{dlAcctB}}}} + out, err := dlCall(t, other, perms, nil, nil) + require.NoError(t, err) + assert.Equal(t, []string{dlID2}, dlIDs(t, out)) + }) +} + +// A user API key is judged on its own permissions, not its owner's. +func TestListRIExchangeDirectExecutions_UserAPIKeyIsNarrowerThanItsOwner(t *testing.T) { + store := dlStore( + dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1), + dlRow(dlID2, dlPtr(dlAcctA), "us-east-1", "unknown", 2), + ) + mockAuth := new(MockAuthService) + mockAuth.On("HasAPIKeyPermissionAPI", mock.Anything, "user-key", "execute", "ri-exchange").Return("u2", "key-1", true, nil) + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, "u2").Return([]string(nil), nil) + inRegion := func(region string) any { + return mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { + return len(sets) == 1 && len(sets[0].Regions) == 1 && sets[0].Regions[0] == region + }) + } + mockAuth.On("HasAPIKeyPermissionForConstraintsAPI", mock.Anything, "key-1", "u2", "execute", "ri-exchange", inRegion("us-east-1")).Return(true, nil) + mockAuth.On("HasAPIKeyPermissionForConstraintsAPI", mock.Anything, "key-1", "u2", "execute", "ri-exchange", inRegion("eu-west-1")).Return(false, nil) + h := &Handler{config: store, auth: mockAuth} + ctx := contextWithPrincipal(context.Background(), &Principal{Kind: PrincipalUserAPIKey, UserID: "u2", APIKeyID: "key-1"}) + + out, err := h.listRIExchangeDirectExecutions(ctx, &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "user-key"}}) + + require.NoError(t, err) + assert.Equal(t, []string{dlID2}, dlIDs(t, out)) + mockAuth.AssertNotCalled(t, "HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +// Rows of a denied region sit between visible ones: every returned row and +// every cursor belongs to a visible pair, and paging is complete. +func TestListRIExchangeDirectExecutions_CursorNeverPointsAtADeniedRow(t *testing.T) { + store := dlStore( + dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1), + dlRow(dlID2, dlPtr(dlAcctA), "us-east-1", "unknown", 2), + dlRow(dlID3, dlPtr(dlAcctA), "eu-west-1", "unknown", 3), + dlRow(dlID4, dlPtr(dlAcctA), "us-east-1", "unknown", 4), + ) + denied := map[string]bool{dlID2: true, dlID4: true} + var got []string + query := map[string]string{"limit": "1"} + for page := 0; page < 5; page++ { + out, err := dlCall(t, store, regionGrant("eu-west-1"), nil, query) + require.NoError(t, err) + got = append(got, dlIDs(t, out)...) + next := out.(riExchangeDirectListResponse).NextCursor + if next == "" { + break + } + cur, err := decodeRIExchangeDirectCursor(next) + require.NoError(t, err) + assert.False(t, denied[cur.ID], "the cursor points at a row the caller cannot see") + query = map[string]string{"limit": "1", "cursor": next} + } + assert.Equal(t, []string{dlID1, dlID3}, got) +} + +func TestListRIExchangeDirectExecutions_NoStore503(t *testing.T) { + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "tok").Return(&Session{UserID: apgUserID}, nil).Maybe() + mockAuth.grantPermissions(riExecute) + h := &Handler{config: new(MockConfigStore), auth: mockAuth} + _, err := h.listRIExchangeDirectExecutions(context.Background(), + &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer tok"}}) + assert.Equal(t, 503, clientCode(t, err)) +} From 1c3148de9276e30307218ed795d237e46614417b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 8 Nov 2026 10:25:36 +0100 Subject: [PATCH 12/30] test(api): fail the direct-exchange list closed on store and auth errors A store or authorization error must fail the list, never return an empty page. Refs #1836 Co-Authored-By: Claude Opus 5.5 --- ...ri_exchange_direct_list_failclosed_test.go | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 internal/api/handler_ri_exchange_direct_list_failclosed_test.go diff --git a/internal/api/handler_ri_exchange_direct_list_failclosed_test.go b/internal/api/handler_ri_exchange_direct_list_failclosed_test.go new file mode 100644 index 000000000..b75aeba3e --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_list_failclosed_test.go @@ -0,0 +1,81 @@ +package api + +import ( + "context" + "errors" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +type failingDirectListStore struct { + *directListStore + pairsErr, listErr error +} + +func (s *failingDirectListStore) ListUnresolvedRIExchangeDirectAccountRegions(ctx context.Context) ([]config.RIExchangeDirectAccountRegion, error) { + if s.pairsErr != nil { + return nil, s.pairsErr + } + return s.directListStore.ListUnresolvedRIExchangeDirectAccountRegions(ctx) +} + +func (s *failingDirectListStore) ListUnresolvedRIExchangeDirectExecutions(ctx context.Context, f config.RIExchangeDirectListFilter) (*config.RIExchangeDirectListPage, error) { + if s.listErr != nil { + return nil, s.listErr + } + return s.directListStore.ListUnresolvedRIExchangeDirectExecutions(ctx, f) +} + +// A failure anywhere in the visibility chain is a server error with no +// partial list: nothing is shown on a guess. +func TestListRIExchangeDirectExecutions_FailsClosed(t *testing.T) { + rows := []config.RIExchangeDirectRow{ + dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1), + dlRow(dlID2, dlPtr(dlAcctB), "eu-west-1", "unknown", 2), + } + boom := errors.New("backend down") + call := func(t *testing.T, store *failingDirectListStore, authErr error) (any, error) { + t.Helper() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "tok").Return(&Session{UserID: apgUserID}, nil).Maybe() + mockAuth.On("HasPermissionAPI", mock.Anything, mock.Anything, "execute", "ri-exchange").Return(true, nil).Maybe() + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return([]string(nil), nil).Maybe() + if authErr != nil { + mockAuth.On("HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, "execute", "ri-exchange", mock.Anything).Return(false, authErr) + } else { + mockAuth.On("HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, "execute", "ri-exchange", mock.Anything).Return(true, nil).Maybe() + } + h := &Handler{config: store, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer tok"}} + return h.listRIExchangeDirectExecutions(context.Background(), req) + } + tests := []struct { + name string + store *failingDirectListStore + auth error + }{ + {"the pair query fails", &failingDirectListStore{directListStore: dlStore(rows...), pairsErr: boom}, nil}, + {"the page query fails", &failingDirectListStore{directListStore: dlStore(rows...), listErr: boom}, nil}, + {"the constraint check fails with a non-403 error", &failingDirectListStore{directListStore: dlStore(rows...)}, boom}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + out, err := call(t, tc.store, tc.auth) + require.Error(t, err) + _, isClient := IsClientError(err) + assert.False(t, isClient, "a backend failure is a 5xx, not a refusal: %v", err) + assert.Nil(t, out, "no partial list") + }) + } + t.Run("control: the same setup lists both rows", func(t *testing.T) { + out, err := call(t, &failingDirectListStore{directListStore: dlStore(rows...)}, nil) + require.NoError(t, err) + assert.Equal(t, []string{dlID1, dlID2}, dlIDs(t, out)) + }) +} From f9491419f8dd2de9481fa2a72ca34ae1a178b13b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 8 Nov 2026 09:26:12 +0100 Subject: [PATCH 13/30] feat(api): route and document the direct-exchange list GET /api/ri-exchange/direct-executions is session-authenticated and documented in openapi.yaml with its filters, paging and the fields it never returns. Refs #1836 Co-Authored-By: Claude Opus 5.5 --- ...dler_ri_exchange_direct_list_route_test.go | 20 +++ internal/api/openapi.yaml | 125 ++++++++++++++++++ internal/api/router.go | 6 + 3 files changed, 151 insertions(+) create mode 100644 internal/api/handler_ri_exchange_direct_list_route_test.go diff --git a/internal/api/handler_ri_exchange_direct_list_route_test.go b/internal/api/handler_ri_exchange_direct_list_route_test.go new file mode 100644 index 000000000..1e3c1dd24 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_list_route_test.go @@ -0,0 +1,20 @@ +package api + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// The list route is session-authenticated, never public, and read-only. +func TestListRIExchangeDirectExecutions_RouteIsAuthUserGET(t *testing.T) { + r := NewRouter(&Handler{}) + for _, route := range r.routes { + if route.ExactPath == "/api/ri-exchange/direct-executions" { + assert.Equal(t, "GET", route.Method) + assert.Equal(t, AuthUser, route.Auth) + return + } + } + t.Fatal("GET /api/ri-exchange/direct-executions is not registered") +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 9dde67325..8cd195cf5 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2881,6 +2881,131 @@ paths: '500': $ref: '#/components/responses/InternalError' + /api/ri-exchange/direct-executions: + get: + operationId: listRIExchangeDirectExecutions + tags: [RIExchange] + summary: List AWS direct RI exchanges that still hold their source RIs + description: > + An AWS direct execute whose outcome was never recorded stays `submitted` + or `unknown`, and the in-flight guard then refuses every later exchange + of its source RIs. This lists those unresolved rows, oldest first, so an + operator can check AWS and resolve them. Requires `execute:ri-exchange`. + A row is listed only when the session's allowed accounts cover its + account (a row with no registered account needs an unrestricted + session) and the grant on `execute:ri-exchange` covers its account and + region, evaluated as the direct execute evaluates it. Rows outside that + are absent, not an error, so an empty list can mean nothing is stuck or + nothing is visible. `error_summary` is a sanitized category message; the + stored provider text, the executing user id and the API key id are never + returned. `resolvable_at` is the earliest time the store would accept a + resolve: ten minutes after creation for a `submitted` row, one minute + after completion for an `unknown` row. + parameters: + - name: status + in: query + required: false + schema: + type: string + enum: [submitted, unknown] + description: Default is both. + - name: ri_id + in: query + required: false + schema: + type: string + maxLength: 200 + description: Source RI id, matched trimmed and case-insensitively. + - name: region + in: query + required: false + schema: + type: string + pattern: '^[a-z0-9-]{1,32}$' + - name: limit + in: query + required: false + schema: + type: integer + minimum: 1 + maximum: 200 + default: 50 + - name: cursor + in: query + required: false + schema: + type: string + description: The `next_cursor` of the previous page. Opaque. + responses: + '200': + description: One page of unresolved direct exchanges + content: + application/json: + schema: + type: object + required: [executions, next_cursor] + properties: + executions: + type: array + items: + type: object + required: [id, status, provider, region, source_ids, targets, max_payment_due, max_payment_currency, created_at, executed_by, error_summary, resolvable, resolvable_at] + properties: + id: + type: string + format: uuid + status: + type: string + enum: [submitted, unknown] + provider: + type: string + enum: [aws] + region: + type: string + source_ids: + type: array + items: + type: string + targets: + type: array + items: + type: object + max_payment_due: + type: string + max_payment_currency: + type: string + created_at: + type: string + format: date-time + executed_by: + type: string + error_summary: + type: string + resolvable: + type: boolean + resolvable_at: + type: string + format: date-time + next_cursor: + type: string + description: Empty at the end of the list. + '400': + description: Invalid `status`, `region`, `ri_id`, `limit` or `cursor`. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '401': + $ref: '#/components/responses/Unauthorized' + '403': + $ref: '#/components/responses/Forbidden' + '503': + description: RI exchange resolve storage is unavailable. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + /api/ri-exchange/resolve/{id}: parameters: - $ref: '#/components/parameters/ResourceID' diff --git a/internal/api/router.go b/internal/api/router.go index 5bb2f961b..14fc1af3b 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -334,6 +334,8 @@ func (r *Router) registerRoutes() { // Operator resolve of a stale processing exchange (#692). AuthUser, so the // outer middleware enforces the session and CSRF. {PathPrefix: "/api/ri-exchange/resolve/", Method: "POST", Handler: r.resolveRIExchangeHandler, Auth: AuthUser}, + // Operator list of AWS direct-execute rows that still hold their source RIs (#1836). + {ExactPath: "/api/ri-exchange/direct-executions", Method: "GET", Handler: r.listRIExchangeDirectExecutionsHandler, Auth: AuthUser}, // Commitment Laddering endpoints (flag-gated default-off, issue #1336). // GET returns all per-account ladder configs; PUT inserts or updates one. @@ -925,6 +927,10 @@ func (r *Router) rejectRIExchangeHandler(ctx context.Context, req *events.Lambda return r.h.rejectRIExchange(ctx, params["id"], req.QueryStringParameters["token"]) } +func (r *Router) listRIExchangeDirectExecutionsHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, _ map[string]string) (any, error) { + return r.h.listRIExchangeDirectExecutions(ctx, req) +} + func (r *Router) resolveRIExchangeHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { return r.h.resolveRIExchange(ctx, req, params["id"]) } From ce3cbff91dc18843b7f4c5698ec2f03c1b475762 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 8 Nov 2026 09:54:44 +0100 Subject: [PATCH 14/30] feat(api): resolve stuck AWS direct RI exchanges by row Add the handler behind POST /api/ri-exchange/direct-executions/{id}/resolve. It reads the row first and authorizes against the row's own account (or the unattributed sentinel), region and max_payment_due, so a grant for another account or region is refused and an out-of-scope row reads as missing. The admin API key is refused because the resolver is stamped on the row. The body is validated (outcome, expected_status, reason, cloud_check, ack for failed) and the built note must fit the store's byte limit. Store errors map to 400, 404 and 409. The route and openapi follow in the next commit. Refs #1836 Co-Authored-By: Claude Opus 5.5 --- .../api/handler_ri_exchange_direct_resolve.go | 95 +++++++++ ...dler_ri_exchange_direct_resolve_request.go | 102 ++++++++++ ...handler_ri_exchange_direct_resolve_test.go | 188 ++++++++++++++++++ internal/api/handler_ri_exchange_resolve.go | 14 +- 4 files changed, 397 insertions(+), 2 deletions(-) create mode 100644 internal/api/handler_ri_exchange_direct_resolve_request.go create mode 100644 internal/api/handler_ri_exchange_direct_resolve_test.go diff --git a/internal/api/handler_ri_exchange_direct_resolve.go b/internal/api/handler_ri_exchange_direct_resolve.go index df5288b22..e34afa2c1 100644 --- a/internal/api/handler_ri_exchange_direct_resolve.go +++ b/internal/api/handler_ri_exchange_direct_resolve.go @@ -1,6 +1,7 @@ package api import ( + "context" "encoding/base64" "encoding/json" "fmt" @@ -9,8 +10,10 @@ import ( "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" + "github.com/aws/aws-lambda-go/events" ) // Operator list of AWS direct RI-exchange rows that still hold their source @@ -132,3 +135,95 @@ func resolvableRIExchangeDirectAt(r *config.RIExchangeDirectRow) time.Time { } return r.CompletedAt.Add(config.RIExchangeDirectResolveUnknownCheckAfter).UTC() } + +type riExchangeDirectResolveResponse struct { + ID string `json:"id"` + Status string `json:"status"` + ResolvedStatus string `json:"resolved_status"` + ResolvedAt time.Time `json:"resolved_at"` + Note string `json:"note"` + Replayed bool `json:"replayed"` +} + +// resolveRIExchangeDirectExecution records an operator's outcome for a stuck +// AWS direct row. Authorization runs against the ROW's account, region and +// payment cap, not the deployment's, because the row may belong to another +// account; an unknown, out-of-scope or non-AWS row is the same 404. +func (h *Handler) resolveRIExchangeDirectExecution(ctx context.Context, req *events.LambdaFunctionURLRequest, id string) (any, error) { + session, err := h.requirePermission(ctx, req, "execute", "ri-exchange") + if err != nil { + return nil, err + } + // The resolver is stamped on the row, so it needs an identified person. + if session.UserID == apiKeyAdminUserID { + return nil, NewClientError(403, "resolving an RI exchange needs a signed-in user or a user API key") + } + if idErr := validateUUID(id); idErr != nil { + return nil, idErr + } + store, ok := h.config.(config.RIExchangeDirectResolveStore) + if !ok { + return nil, NewClientError(503, "RI exchange resolve storage is unavailable") + } + row, err := store.GetRIExchangeDirectExecution(ctx, id) + if mapped := mapRIExchangeResolveError(err); mapped != nil { + return nil, mapped + } + if authErr := h.authorizeRIExchangeDirectRow(ctx, session, row); authErr != nil { + return nil, authErr + } + resolve, err := buildRIExchangeDirectResolve(session, row, req.Body) + if err != nil { + return nil, err + } + res, err := store.ResolveRIExchangeDirectExecution(ctx, resolve) + if mapped := mapRIExchangeResolveError(err); mapped != nil { + return nil, mapped + } + if !res.Replayed { + logging.Warnf("RI exchange direct row resolved by operator: id=%s actor=%q via=%s outcome=%s", + id, session.UserID, credentialKind(ctx, session), res.ResolvedStatus) + } + return riExchangeDirectResolveResponse{ID: res.ID, Status: res.Status, ResolvedStatus: res.ResolvedStatus, + ResolvedAt: res.ResolvedAt.UTC(), Note: res.Note, Replayed: res.Replayed}, nil +} + +// buildRIExchangeDirectResolve validates the body against the row and builds +// the store request. The note must fit the store's byte limit, so an overlong +// reason is a clear 400 here rather than the store's. +func buildRIExchangeDirectResolve(session *Session, row *config.RIExchangeDirectRow, raw string) (config.RIExchangeDirectResolve, error) { + body, checkedAt, err := decodeRIExchangeDirectResolve(raw, row.Region) + if err != nil { + return config.RIExchangeDirectResolve{}, err + } + note := body.note(checkedAt) + if len(note) > config.RIExchangeDirectResolveNoteMax { + return config.RIExchangeDirectResolve{}, NewClientError(400, "reason too long once encoded; shorten it") + } + return config.RIExchangeDirectResolve{ + ID: row.ID, Outcome: body.Outcome, ExpectedStatus: body.ExpectedStatus, Note: note, + CheckedAt: checkedAt, ResolvedByUserID: resolveCreatorUserID(session), ResolvedBy: fourEyesActorIdentity(session), + }, nil +} + +// authorizeRIExchangeDirectRow applies the session's account scope and the +// grant on execute:ri-exchange to the row's own account, region and +// max_payment_due (the most the original request could have committed). A +// row outside the account scope reads as missing. +func (h *Handler) authorizeRIExchangeDirectRow(ctx context.Context, session *Session, row *config.RIExchangeDirectRow) error { + scope, err := h.getAccountScope(ctx, session) + if err != nil { + return fmt.Errorf("failed to get allowed accounts: %w", err) + } + account, inScope := riExchangeDirectPairAccount(scope, config.RIExchangeDirectAccountRegion{CloudAccountID: row.CloudAccountID, Region: row.Region}) + if !inScope { + return errNotFound + } + return h.requireRIExchangeConstraintsFor(ctx, session, account, row.Region, func() (float64, error) { + due, ok := parseRIExchangeAmount(row.MaxPaymentDue) + if !ok { + return 0, fmt.Errorf("unreadable payment cap on direct exchange %s", row.ID) + } + return due, nil + }) +} diff --git a/internal/api/handler_ri_exchange_direct_resolve_request.go b/internal/api/handler_ri_exchange_direct_resolve_request.go new file mode 100644 index 000000000..d87e9e4d4 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_resolve_request.go @@ -0,0 +1,102 @@ +package api + +import ( + "encoding/json" + "fmt" + "io" + "strings" + "time" + "unicode/utf8" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// riExchangeDirectResolveRequest is the body of POST +// /api/ri-exchange/direct-executions/{id}/resolve. expected_status is the +// status the operator saw in the list; outcome "failed" releases the source +// RIs and needs acknowledge_unknown_outcome. +type riExchangeDirectResolveRequest struct { + Outcome string `json:"outcome"` + ExpectedStatus string `json:"expected_status"` + Reason string `json:"reason"` + Acknowledge bool `json:"acknowledge_unknown_outcome"` + CloudCheck *riExchangeDirectResolveCloudCheck `json:"cloud_check"` +} + +type riExchangeDirectResolveCloudCheck struct { + Region string `json:"checked_region"` + CheckedAt string `json:"checked_at"` +} + +// decodeRIExchangeDirectResolve reads one JSON document and checks its shape +// against the row being resolved. Which times the store admits is decided +// there, on the database clock. +func decodeRIExchangeDirectResolve(raw, rowRegion string) (riExchangeDirectResolveRequest, time.Time, error) { + var body riExchangeDirectResolveRequest + if len(raw) > riExchangeResolveBodyMax { + return body, time.Time{}, NewClientError(400, "RI exchange resolve body is too large") + } + decoder := json.NewDecoder(strings.NewReader(raw)) + decoder.DisallowUnknownFields() + if err := decoder.Decode(&body); err != nil { + return body, time.Time{}, NewClientError(400, "Invalid RI exchange resolve body") + } + if err := decoder.Decode(new(any)); err != io.EOF { + return body, time.Time{}, NewClientError(400, "Expected one RI exchange resolve body") + } + body.Reason = strings.TrimSpace(body.Reason) + if err := body.validate(); err != nil { + return body, time.Time{}, err + } + at, err := body.checkedAt(rowRegion) + return body, at, err +} + +func (b riExchangeDirectResolveRequest) validate() error { + switch { + case b.Outcome != config.RIExchangeDirectStatusFailed && b.Outcome != config.RIExchangeDirectStatusCompleted: + return NewClientError(400, `outcome must be "failed" or "completed"`) + case b.ExpectedStatus != config.RIExchangeDirectStatusSubmitted && b.ExpectedStatus != config.RIExchangeDirectStatusUnknown: + return NewClientError(400, `expected_status must be "submitted" or "unknown"`) + case utf8.RuneCountInString(b.Reason) < riExchangeResolveReasonMin || utf8.RuneCountInString(b.Reason) > riExchangeResolveReasonMax: + return NewClientError(400, fmt.Sprintf("reason must be %d to %d characters", riExchangeResolveReasonMin, riExchangeResolveReasonMax)) + case hasControlRune(b.Reason): + return NewClientError(400, "reason must not contain control characters") + } + return b.validateCheckAndAck() +} + +func (b riExchangeDirectResolveRequest) validateCheckAndAck() error { + switch { + case b.CloudCheck == nil: + return NewClientError(400, "cloud_check is required: say where you verified the exchange at AWS") + case b.Outcome == config.RIExchangeDirectStatusFailed && !b.Acknowledge: + return NewClientError(400, "acknowledge_unknown_outcome is required to release the source RIs: CUDly cannot see whether the exchange happened") + } + return nil +} + +// checkedAt validates the shape of the operator's AWS-side check: the region +// it names must be the row's own. +func (b riExchangeDirectResolveRequest) checkedAt(rowRegion string) (time.Time, error) { + c := b.CloudCheck + if !riExchangeResolveRegionRe.MatchString(c.Region) || c.Region != rowRegion { + return time.Time{}, NewClientError(400, "checked_region must be the region of the exchange") + } + at, err := time.Parse(time.RFC3339, c.CheckedAt) + if err != nil { + return time.Time{}, NewClientError(400, "checked_at must be an RFC 3339 timestamp") + } + return at.UTC(), nil +} + +// note is the exact text stored in resolution_note. The fields are in a fixed +// order and quoted, so a repeat of the same request produces the same string +// and a different one cannot. +func (b riExchangeDirectResolveRequest) note(checkedAt time.Time) string { + s := fmt.Sprintf("operator:%s: region=%q checked_at=%s", b.Outcome, b.CloudCheck.Region, checkedAt.Format(time.RFC3339)) + if b.Outcome == config.RIExchangeDirectStatusFailed { + s += " ack=true" + } + return s + fmt.Sprintf(" reason=%q", b.Reason) +} diff --git a/internal/api/handler_ri_exchange_direct_resolve_test.go b/internal/api/handler_ri_exchange_direct_resolve_test.go new file mode 100644 index 000000000..396b73c72 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_resolve_test.go @@ -0,0 +1,188 @@ +package api + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Handler tests for POST /api/ri-exchange/direct-executions/{id}/resolve +// (#1836). The store is a FIXTURE; the row lock, the settle age and the +// checked_at bounds are covered against real PostgreSQL in internal/config. + +const ( + drRegion = "eu-west-1" + // drOtherID is a second stuck row. Nothing returned for the first may name it. + drOtherID = dlID4 +) + +var drResolvedAt = time.Date(2026, 11, 8, 9, 0, 0, 0, time.UTC) + +type directResolveStore struct { + *directListStore + rows map[string]config.RIExchangeDirectRow + called int + got config.RIExchangeDirectResolve + res *config.RIExchangeDirectResolveResult + err error +} + +func (s *directResolveStore) GetRIExchangeDirectExecution(_ context.Context, id string) (*config.RIExchangeDirectRow, error) { + r, ok := s.rows[id] + if !ok { + return nil, config.ErrRIExchangeResolveNotFound + } + return &r, nil +} + +func (s *directResolveStore) ResolveRIExchangeDirectExecution(_ context.Context, r config.RIExchangeDirectResolve) (*config.RIExchangeDirectResolveResult, error) { + s.called++ + s.got = r + if s.res != nil || s.err != nil { + return s.res, s.err + } + return &config.RIExchangeDirectResolveResult{ID: r.ID, Status: r.ExpectedStatus, ResolvedStatus: r.Outcome, + ResolvedAt: drResolvedAt, Note: r.Note}, nil +} + +// drStore holds a stuck row in drRegion for account A, and a second row of +// account B whose id must never appear in a response for the first. +func drStore(rows ...config.RIExchangeDirectRow) *directResolveStore { + if len(rows) == 0 { + rows = []config.RIExchangeDirectRow{dlRow(dlID1, dlPtr(dlAcctA), drRegion, "unknown", 1)} + } + s := &directResolveStore{directListStore: dlStore(), rows: map[string]config.RIExchangeDirectRow{}} + for _, r := range rows { + s.rows[r.ID] = r + } + if _, ok := s.rows[drOtherID]; !ok { + s.rows[drOtherID] = dlRow(drOtherID, dlPtr(dlAcctB), drRegion, "unknown", 2) + } + return s +} + +// drBody is a valid request for an unknown row in drRegion. +func drBody(outcome, extra string) string { + return fmt.Sprintf(`{"outcome":%q,"expected_status":"unknown","reason":"source RIs still active in the EC2 console"%s,`+ + `"cloud_check":{"checked_region":%q,"checked_at":%q}}`, + outcome, extra, drRegion, riResolveAt.Format(time.RFC3339)) +} + +var drFailedBody = drBody("failed", `,"acknowledge_unknown_outcome":true`) + +type drCaller struct { + perms []auth.Permission + accounts []string // nil means unrestricted + adminKey bool + email string +} + +func (c drCaller) call(store *directResolveStore, id, body string) (any, error) { + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "tok").Return(&Session{UserID: apgUserID, Email: c.email}, nil).Maybe() + mockAuth.grantPermissionsScoped(c.perms, c.accounts) + h := &Handler{config: store, auth: mockAuth, + // The deployment's account is a third one: only the ROW's account may + // decide authorization. + reshapeAccountResolver: func(context.Context) (string, error) { return "33333333-3333-4333-8333-333333333333", nil }} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer tok"}, Body: body} + if c.adminKey { + h.apiKey = apgAdminKy + req.Headers = map[string]string{"x-api-key": apgAdminKy} + } + return h.resolveRIExchangeDirectExecution(context.Background(), req, id) +} + +func TestResolveRIExchangeDirect_PassesOperatorDecisionToStore(t *testing.T) { + logs := captureLogs(t) + store := drStore() + out, err := drCaller{perms: riExecute, email: "ops@example.com"}.call(store, dlID1, drFailedBody) + require.NoError(t, err) + require.Equal(t, 1, store.called) + got := store.got + assert.Equal(t, dlID1, got.ID) + assert.Equal(t, config.RIExchangeDirectStatusFailed, got.Outcome) + assert.Equal(t, config.RIExchangeDirectStatusUnknown, got.ExpectedStatus) + assert.Equal(t, riResolveAt, got.CheckedAt) + require.NotNil(t, got.ResolvedByUserID) + assert.Equal(t, apgUserID, *got.ResolvedByUserID) + assert.Equal(t, "ops@example.com", got.ResolvedBy) + assert.Equal(t, `operator:failed: region="eu-west-1" checked_at=2026-10-19T08:00:00Z ack=true reason="source RIs still active in the EC2 console"`, got.Note) + resp, ok := out.(riExchangeDirectResolveResponse) + require.True(t, ok) + assert.Equal(t, riExchangeDirectResolveResponse{ID: dlID1, Status: "unknown", ResolvedStatus: "failed", + ResolvedAt: drResolvedAt, Note: got.Note}, resp) + for _, want := range []string{dlID1, apgUserID, "outcome=failed", "via="} { + assert.Contains(t, logs.String(), want) + } + + completed := drStore() + _, err = drCaller{perms: riExecute}.call(completed, dlID1, drBody("completed", "")) + require.NoError(t, err) + assert.Equal(t, `operator:completed: region="eu-west-1" checked_at=2026-10-19T08:00:00Z reason="source RIs still active in the EC2 console"`, completed.got.Note) +} + +func TestResolveRIExchangeDirect_ReplayIsReportedAndNotLogged(t *testing.T) { + logs := captureLogs(t) + store := drStore() + store.res = &config.RIExchangeDirectResolveResult{ID: dlID1, Status: "unknown", ResolvedStatus: "failed", + ResolvedAt: drResolvedAt, Note: "n", Replayed: true} + out, err := drCaller{perms: riExecute}.call(store, dlID1, drFailedBody) + require.NoError(t, err) + assert.True(t, out.(riExchangeDirectResolveResponse).Replayed) + assert.NotContains(t, logs.String(), "resolved by operator", "a replay wrote nothing, so it logs no resolution") +} + +func TestResolveRIExchangeDirect_MapsStoreOutcomes(t *testing.T) { + tests := []struct { + name string + err error + want int + }{ + {"missing at resolve time", config.ErrRIExchangeResolveNotFound, 404}, + {"checked_at outside the window", fmt.Errorf("%w: checked_at is in the future", config.ErrRIExchangeResolveInvalid), 400}, + {"already settled", fmt.Errorf("%w: the exchange is already completed", config.ErrRIExchangeResolveConflict), 409}, + {"may still be in flight", fmt.Errorf("%w: the exchange may still be in flight; wait until 2026-11-08T09:10:00Z", config.ErrRIExchangeResolveConflict), 409}, + {"already resolved differently", fmt.Errorf("%w: the exchange was already resolved as failed", config.ErrRIExchangeResolveConflict), 409}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + store := drStore() + store.err = tc.err + _, err := drCaller{perms: riExecute}.call(store, dlID1, drFailedBody) + if tc.want == 404 { + require.ErrorIs(t, err, errNotFound) + return + } + assert.Equal(t, tc.want, clientCode(t, err)) + }) + } + t.Run("an unexpected store failure is not a client error", func(t *testing.T) { + store := drStore() + store.err = errors.New("connection reset") + _, err := drCaller{perms: riExecute}.call(store, dlID1, drFailedBody) + require.Error(t, err) + _, isClient := IsClientError(err) + assert.False(t, isClient) + }) +} + +func TestResolveRIExchangeDirect_Needs503WithoutTheStore(t *testing.T) { + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "tok").Return(&Session{UserID: apgUserID}, nil).Maybe() + mockAuth.grantPermissions(riExecute) + h := &Handler{config: new(MockConfigStore), auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer tok"}, Body: drFailedBody} + _, err := h.resolveRIExchangeDirectExecution(context.Background(), req, dlID1) + assert.Equal(t, 503, clientCode(t, err)) +} diff --git a/internal/api/handler_ri_exchange_resolve.go b/internal/api/handler_ri_exchange_resolve.go index da270fde5..65db477ab 100644 --- a/internal/api/handler_ri_exchange_resolve.go +++ b/internal/api/handler_ri_exchange_resolve.go @@ -165,13 +165,23 @@ func (h *Handler) riExchangeHoldAmount(ctx context.Context, id string) (float64, if record == nil { return 0, errNotFound } - due, err := strconv.ParseFloat(record.PaymentDue, 64) - if err != nil || math.IsNaN(due) || math.IsInf(due, 0) || due < 0 { + due, ok := parseRIExchangeAmount(record.PaymentDue) + if !ok { return 0, fmt.Errorf("unreadable hold amount on exchange %s", id) } return due, nil } +// parseRIExchangeAmount reads a stored amount, refusing a value that is not a +// finite non-negative number. +func parseRIExchangeAmount(s string) (float64, bool) { + due, err := strconv.ParseFloat(s, 64) + if err != nil || math.IsNaN(due) || math.IsInf(due, 0) || due < 0 { + return 0, false + } + return due, true +} + func mapRIExchangeResolveError(err error) error { switch { case err == nil: From 9aebc46bb02503b5879b9daf587c77a196d6a612 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 8 Nov 2026 09:58:15 +0100 Subject: [PATCH 15/30] test(api): pin the direct-row resolve body validation Table of invalid bodies (unknown field, two documents, oversized, outcome, expected_status, reason length in runes, control runes, missing cloud_check, failed without the acknowledgement, wrong or malformed region, bad checked_at) that must be refused with 400 before the store. Also pins the reason boundaries, a reason whose encoded note overflows the store's byte limit, and that completed needs no acknowledgement. Refs #1836 Co-Authored-By: Claude Opus 5.5 --- ...er_ri_exchange_direct_resolve_body_test.go | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 internal/api/handler_ri_exchange_direct_resolve_body_test.go diff --git a/internal/api/handler_ri_exchange_direct_resolve_body_test.go b/internal/api/handler_ri_exchange_direct_resolve_body_test.go new file mode 100644 index 000000000..62f75f1f3 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_resolve_body_test.go @@ -0,0 +1,83 @@ +package api + +import ( + "fmt" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Body validation for the direct-row resolve. Every case is a 400 that never +// reaches the store. +func TestResolveRIExchangeDirect_BodyValidation(t *testing.T) { + check := func(region, at string) string { + return fmt.Sprintf(`"cloud_check":{"checked_region":%q,"checked_at":%q}`, region, at) + } + good := check(drRegion, "2026-10-19T08:00:00Z") + reason := `"reason":"source RIs still active in the EC2 console"` + head := `{"outcome":"failed","expected_status":"unknown",` + reason + `,"acknowledge_unknown_outcome":true,` + for name, body := range map[string]string{ + "empty": ``, + "unknown field": head + good + `,"exchange_id":"x"}`, + "two documents": drFailedBody + drFailedBody, + "oversized body": `{"outcome":"completed","reason":"` + strings.Repeat("x", 5000) + `"}`, + "bad outcome": `{"outcome":"canceled","expected_status":"unknown",` + reason + `,` + good + `}`, + "missing outcome": `{"expected_status":"unknown",` + reason + `,` + good + `}`, + "missing expected status": `{"outcome":"completed",` + reason + `,` + good + `}`, + "bad expected status": `{"outcome":"completed","expected_status":"completed",` + reason + `,` + good + `}`, + "short reason (9 runes)": `{"outcome":"completed","expected_status":"unknown","reason":"123456789",` + good + `}`, + "whitespace reason": `{"outcome":"completed","expected_status":"unknown","reason":" ",` + good + `}`, + "long reason (301 runes)": `{"outcome":"completed","expected_status":"unknown","reason":"` + strings.Repeat("é", 301) + `",` + good + `}`, + "300 emoji overflow the note's byte limit": `{"outcome":"completed","expected_status":"unknown","reason":"` + strings.Repeat("😀", 300) + `",` + good + `}`, + "control char in reason": `{"outcome":"completed","expected_status":"unknown","reason":"source RIs\nstill active",` + good + `}`, + "missing cloud check": `{"outcome":"completed","expected_status":"unknown",` + reason + `}`, + "cloud check null": head + `"cloud_check":null}`, + "failed without ack": `{"outcome":"failed","expected_status":"unknown",` + reason + `,` + good + `}`, + "failed with ack false": `{"outcome":"failed","expected_status":"unknown",` + reason + `,"acknowledge_unknown_outcome":false,` + good + `}`, + "wrong region": head + check("us-east-1", "2026-10-19T08:00:00Z") + `}`, + "malformed region": head + check("EU WEST", "2026-10-19T08:00:00Z") + `}`, + "empty region": head + check("", "2026-10-19T08:00:00Z") + `}`, + "bad checked_at": head + check(drRegion, "yesterday") + `}`, + "missing checked_at": head + `"cloud_check":{"checked_region":"eu-west-1"}}`, + "account in the cloud check": head + `"cloud_check":{"checked_region":"eu-west-1","checked_account_id":"123","checked_at":"2026-10-19T08:00:00Z"}}`, + } { + t.Run(name, func(t *testing.T) { + store := drStore() + _, err := drCaller{perms: riExecute}.call(store, dlID1, body) + assert.Equal(t, 400, clientCode(t, err)) + assert.Zero(t, store.called, "an invalid body must not reach the store") + }) + } +} + +// Reasons at the rune boundaries are accepted, and the stored note quotes them. +func TestResolveRIExchangeDirect_ReasonBoundaries(t *testing.T) { + for name, reason := range map[string]string{ + "10 runes": strings.Repeat("é", 10), + "300 runes": strings.Repeat("é", 300), + "trimmed to 10 runes": " " + strings.Repeat("a", 10) + " ", + "a quote is escaped once": `say "ok" to AWS`, + } { + t.Run(name, func(t *testing.T) { + store := drStore() + body := fmt.Sprintf(`{"outcome":"completed","expected_status":"unknown","reason":%q,"cloud_check":{"checked_region":%q,"checked_at":"2026-10-19T08:00:00Z"}}`, reason, drRegion) + _, err := drCaller{perms: riExecute}.call(store, dlID1, body) + require.NoError(t, err) + assert.Contains(t, store.got.Note, fmt.Sprintf("reason=%q", strings.TrimSpace(reason))) + }) + } +} + +// completed does not need the acknowledgement; failed with it passes, and the +// note records it. +func TestResolveRIExchangeDirect_AckOnlyForFailed(t *testing.T) { + store := drStore() + _, err := drCaller{perms: riExecute}.call(store, dlID1, drBody("completed", "")) + require.NoError(t, err) + assert.NotContains(t, store.got.Note, "ack=true") + assert.Equal(t, config.RIExchangeDirectStatusCompleted, store.got.Outcome) +} From 13a49b664847d8f76f05bce835a8c61130d80133 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 8 Nov 2026 09:59:58 +0100 Subject: [PATCH 16/30] feat(api): route and document the direct-row resolve Register POST /api/ri-exchange/direct-executions/{id}/resolve as AuthUser, so the outer middleware enforces the session and CSRF, and document it in openapi. Pin the route's method and auth class, and that the 10-minute settle age exceeds the execution window (deadline, terminal save and a 60-second margin), so a later timeout bump cannot silently open it. Refs #1836 Co-Authored-By: Claude Opus 5.5 --- ...r_ri_exchange_direct_resolve_route_test.go | 30 ++++ internal/api/openapi.yaml | 132 ++++++++++++++++++ internal/api/router.go | 7 + 3 files changed, 169 insertions(+) create mode 100644 internal/api/handler_ri_exchange_direct_resolve_route_test.go diff --git a/internal/api/handler_ri_exchange_direct_resolve_route_test.go b/internal/api/handler_ri_exchange_direct_resolve_route_test.go new file mode 100644 index 000000000..4523d6147 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_resolve_route_test.go @@ -0,0 +1,30 @@ +package api + +import ( + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" +) + +func TestResolveRIExchangeDirect_RouteIsAuthUserPOST(t *testing.T) { + r := NewRouter(&Handler{}) + for _, route := range r.routes { + if route.PathPrefix == "/api/ri-exchange/direct-executions/" && route.PathSuffix == "/resolve" { + assert.Equal(t, "POST", route.Method) + assert.Equal(t, AuthUser, route.Auth, "the outer middleware enforces the session and CSRF") + return + } + } + t.Fatal("POST /api/ri-exchange/direct-executions/{id}/resolve is not registered") +} + +// The settle age rests on a bounded window: the direct execute takes one +// deadline before saving the row, the terminal save gets its own timeout, and +// a margin covers clock skew. A later timeout bump must not silently open the +// window. +func TestResolveRIExchangeDirect_SettleAgeCoversTheExecutionWindow(t *testing.T) { + window := riExchangeExecutionTimeout + riExchangeTerminalSaveTimeout + time.Minute + assert.Greater(t, config.RIExchangeDirectResolveSubmittedAfter, window) +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 8cd195cf5..30131294a 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -3006,6 +3006,138 @@ paths: schema: $ref: '#/components/schemas/Error' + /api/ri-exchange/direct-executions/{id}/resolve: + parameters: + - $ref: '#/components/parameters/ResourceID' + post: + operationId: resolveRIExchangeDirectExecution + tags: [RIExchange] + summary: Record an operator's outcome for a stuck AWS direct RI exchange + description: > + Settles a row from `GET /api/ri-exchange/direct-executions` after the + operator checked AWS. The row keeps its original status and gains the + resolution, and its source RIs stop blocking later exchanges. `failed` + releases them and requires `acknowledge_unknown_outcome`. Requires + `execute:ri-exchange`, checked against the row's own account, region + and `max_payment_due` (not the deployment's), and a signed-in user or + user API key (the admin API key is refused). A `submitted` row is + refused until 10 minutes after it was created, and `checked_at` must + be at or after that time (for an `unknown` row, one minute after it + completed) and not in the future. `expected_status` must be the status + the row has now. A repeat of the same request returns the stored + result with `replayed: true`. A row outside the session's allowed + accounts reads as not found. + parameters: + - $ref: '#/components/parameters/CSRFToken' + requestBody: + required: true + content: + application/json: + schema: + type: object + additionalProperties: false + required: [outcome, expected_status, reason, cloud_check] + properties: + outcome: + type: string + enum: [failed, completed] + expected_status: + type: string + enum: [submitted, unknown] + description: The status the operator saw in the list. + reason: + type: string + minLength: 10 + maxLength: 300 + description: Free text, trimmed, without control characters. + acknowledge_unknown_outcome: + type: boolean + description: Must be true when outcome is failed. + cloud_check: + type: object + additionalProperties: false + required: [checked_region, checked_at] + description: Where and when the operator verified the exchange at AWS. + properties: + checked_region: + type: string + pattern: '^[a-z0-9-]{1,32}$' + description: Must be the region of the exchange. + checked_at: + type: string + format: date-time + responses: + '200': + description: Row resolved, or the same resolve replayed + content: + application/json: + schema: + type: object + required: [id, status, resolved_status, resolved_at, note, replayed] + properties: + id: + type: string + format: uuid + status: + type: string + enum: [submitted, unknown, completed, failed] + description: > + The row's provider status, which resolving leaves + unchanged. A late finalize may have set it to + `completed` or `failed` since. `resolved_status` is the + operator's resolution. + resolved_status: + type: string + enum: [completed, failed] + resolved_at: + type: string + format: date-time + note: + type: string + replayed: + type: boolean + '400': + description: > + Invalid id or body: body over 4096 bytes, unknown fields, a + missing or invalid field, a region other than the row's, or a + `checked_at` outside the allowed window. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '401': + $ref: '#/components/responses/Unauthorized' + '403': + description: > + CSRF validation failed, the session lacks `execute:ri-exchange` or + fails its constraints for the row's account, region or amount, or + the caller is the admin API key. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '404': + description: No such AWS direct row, or its account is outside the session's allowed accounts. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '409': + description: > + The row cannot be resolved this way: already resolved differently, + already settled, its status is not `expected_status`, or a + `submitted` row may still be in flight. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '503': + description: RI exchange resolve storage is unavailable. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + /api/ri-exchange/resolve/{id}: parameters: - $ref: '#/components/parameters/ResourceID' diff --git a/internal/api/router.go b/internal/api/router.go index 14fc1af3b..18d6be8c6 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -336,6 +336,9 @@ func (r *Router) registerRoutes() { {PathPrefix: "/api/ri-exchange/resolve/", Method: "POST", Handler: r.resolveRIExchangeHandler, Auth: AuthUser}, // Operator list of AWS direct-execute rows that still hold their source RIs (#1836). {ExactPath: "/api/ri-exchange/direct-executions", Method: "GET", Handler: r.listRIExchangeDirectExecutionsHandler, Auth: AuthUser}, + // Operator resolve of one such row. AuthUser, so the outer middleware + // enforces the session and CSRF. + {PathPrefix: "/api/ri-exchange/direct-executions/", PathSuffix: "/resolve", Method: "POST", Handler: r.resolveRIExchangeDirectExecutionHandler, Auth: AuthUser}, // Commitment Laddering endpoints (flag-gated default-off, issue #1336). // GET returns all per-account ladder configs; PUT inserts or updates one. @@ -931,6 +934,10 @@ func (r *Router) listRIExchangeDirectExecutionsHandler(ctx context.Context, req return r.h.listRIExchangeDirectExecutions(ctx, req) } +func (r *Router) resolveRIExchangeDirectExecutionHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { + return r.h.resolveRIExchangeDirectExecution(ctx, req, params["id"]) +} + func (r *Router) resolveRIExchangeHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { return r.h.resolveRIExchange(ctx, req, params["id"]) } From ad11f8d158f239afb3115f5b2a3a77b7ab2553af Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 8 Nov 2026 10:08:14 +0100 Subject: [PATCH 17/30] test(api): pin the direct-row resolve authorization The '|| due < 0' check in parseRIExchangeAmount is redundant with auth's negative-amount denial, so a probe removing it survives (an equivalent mutant). Refs #1836 Co-Authored-By: Claude Opus 5.5 --- ...r_ri_exchange_direct_resolve_authz_test.go | 143 ++++++++++++++++++ 1 file changed, 143 insertions(+) create mode 100644 internal/api/handler_ri_exchange_direct_resolve_authz_test.go diff --git a/internal/api/handler_ri_exchange_direct_resolve_authz_test.go b/internal/api/handler_ri_exchange_direct_resolve_authz_test.go new file mode 100644 index 000000000..9502156cc --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_resolve_authz_test.go @@ -0,0 +1,143 @@ +package api + +import ( + "fmt" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestResolveRIExchangeDirect_Authorization(t *testing.T) { + unattributed := dlRow(dlID2, nil, drRegion, "unknown", 3) + tests := []struct { + name string + caller drCaller + id string + want int // 0 = allowed + }{ + {"execute:ri-exchange, unrestricted", drCaller{perms: riExecute}, dlID1, 0}, + {"view-only user", drCaller{perms: []auth.Permission{{Action: auth.ActionView, Resource: auth.ResourcePurchases}}}, dlID1, 403}, + {"admin API key has no identity to stamp", drCaller{adminKey: true}, dlID1, 403}, + {"row account inside the session's accounts", drCaller{perms: riExecute, accounts: []string{dlAcctA}}, dlID1, 0}, + {"row account outside the session's accounts reads as missing", drCaller{perms: riExecute, accounts: []string{dlAcctB}}, dlID1, 404}, + {"unattributed row needs an unrestricted session", drCaller{perms: riExecute, accounts: []string{dlAcctA}}, dlID2, 404}, + {"unattributed row, unrestricted session", drCaller{perms: riExecute}, dlID2, 0}, + {"unknown id", drCaller{perms: riExecute}, dlID3, 404}, + {"id that is not a uuid", drCaller{perms: riExecute}, "not-a-uuid", 400}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + store := drStore(dlRow(dlID1, dlPtr(dlAcctA), drRegion, "unknown", 1), unattributed) + _, err := tc.caller.call(store, tc.id, drFailedBody) + if tc.want == 0 { + require.NoError(t, err) + assert.Equal(t, 1, store.called) + return + } + assert.Equal(t, 0, store.called, "a refused caller must not reach the store") + if tc.want == 404 { + require.ErrorIs(t, err, errNotFound) + return + } + assert.Equal(t, tc.want, clientCode(t, err)) + }) + } +} + +// The grant is judged against the ROW's account, region and payment cap. The +// deployment's own account is a different one in every case. +func TestResolveRIExchangeDirect_ConstraintsUseTheRowsAccountAndRegion(t *testing.T) { + grant := func(c auth.PermissionConstraints) []auth.Permission { + return []auth.Permission{{Action: auth.ActionExecute, Resource: "ri-exchange", Constraints: &c}} + } + unattributed := dlRow(dlID2, nil, drRegion, "unknown", 3) + tests := []struct { + name string + perms []auth.Permission + id string + want int // 0 = allowed + }{ + {"grant for the row's region", grant(auth.PermissionConstraints{Regions: []string{drRegion}}), dlID1, 0}, + {"grant for another region", grant(auth.PermissionConstraints{Regions: []string{"us-east-1"}}), dlID1, 403}, + {"grant for the row's account", grant(auth.PermissionConstraints{AccountIDs: []string{dlAcctA}}), dlID1, 0}, + {"grant for another account", grant(auth.PermissionConstraints{AccountIDs: []string{dlAcctB}}), dlID1, 403}, + {"grant for the deployment's account only", grant(auth.PermissionConstraints{AccountIDs: []string{"33333333-3333-4333-8333-333333333333"}}), dlID1, 403}, + {"account-constrained grant never covers an unattributed row", grant(auth.PermissionConstraints{AccountIDs: []string{dlAcctA}}), dlID2, 403}, + {"cap below the row's max_payment_due (100)", riCapped(99.99), dlID1, 403}, + {"cap equal to it", riCapped(100), dlID1, 0}, + {"cap above it", riCapped(5000), dlID1, 0}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + store := drStore(dlRow(dlID1, dlPtr(dlAcctA), drRegion, "unknown", 1), unattributed) + _, err := drCaller{perms: tc.perms}.call(store, tc.id, drFailedBody) + if tc.want == 0 { + require.NoError(t, err) + assert.Equal(t, 1, store.called) + return + } + assert.Equal(t, tc.want, clientCode(t, err)) + assert.Zero(t, store.called, "a refused caller must not reach the store") + }) + } +} + +func TestResolveRIExchangeDirect_CappedGrantRefusesUnreadableAmount(t *testing.T) { + for _, due := range []string{"", "abc", "-1", "NaN", "+Inf"} { + t.Run(due, func(t *testing.T) { + row := dlRow(dlID1, dlPtr(dlAcctA), drRegion, "unknown", 1) + row.MaxPaymentDue = due + store := drStore(row) + _, err := drCaller{perms: riCapped(5000)}.call(store, dlID1, drFailedBody) + require.Error(t, err) + assert.Zero(t, store.called) + }) + } +} + +// An uncapped grant never reads the amount, so an unreadable one passes. +func TestResolveRIExchangeDirect_UncappedGrantIgnoresTheAmount(t *testing.T) { + row := dlRow(dlID1, dlPtr(dlAcctA), drRegion, "unknown", 1) + row.MaxPaymentDue = "abc" + store := drStore(row) + _, err := drCaller{perms: riExecute}.call(store, dlID1, drFailedBody) + require.NoError(t, err) + assert.Equal(t, 1, store.called) +} + +// No refusal or answer for one row may carry another row's id, and an +// out-of-scope row answers exactly like a missing one. +func TestResolveRIExchangeDirect_ResponsesNameOnlyTheRequestedRow(t *testing.T) { + var seen []string + collect := func(out any, err error) { + if err != nil { + seen = append(seen, err.Error()) + return + } + seen = append(seen, fmt.Sprintf("%+v", out)) + } + for _, err := range []error{ + nil, + fmt.Errorf("%w: the exchange is already completed", config.ErrRIExchangeResolveConflict), + fmt.Errorf("%w: checked_at is in the future", config.ErrRIExchangeResolveInvalid), + config.ErrRIExchangeResolveNotFound, + } { + store := drStore() + store.err = err + collect(drCaller{perms: riExecute}.call(store, dlID1, drFailedBody)) + } + collect(drCaller{perms: riExecute, accounts: []string{dlAcctB}}.call(drStore(), dlID1, drFailedBody)) + collect(drCaller{perms: riExecute}.call(drStore(), dlID3, drFailedBody)) + collect(drCaller{perms: riCapped(1)}.call(drStore(), dlID1, drFailedBody)) + collect(drCaller{perms: riExecute}.call(drStore(), dlID1, `{"outcome":"nope"}`)) + for _, s := range seen { + assert.NotContains(t, s, drOtherID) + } + + _, outErr := drCaller{perms: riExecute, accounts: []string{dlAcctB}}.call(drStore(), dlID1, drFailedBody) + _, missErr := drCaller{perms: riExecute, accounts: []string{dlAcctB}}.call(drStore(), dlID3, drFailedBody) + assert.Equal(t, missErr, outErr, "out of scope must be indistinguishable from missing") +} From 85a3b4e7321f95839997dbc63abe5473f99d2e9c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 8 Nov 2026 10:09:23 +0100 Subject: [PATCH 18/30] test(api): resolve a stuck direct exchange end to end A lost accept leaves an unknown row, the guard refuses the same RI, and the list and resolve handlers release it so direct, AUTO and approve paths proceed. Refs #1836 Co-Authored-By: Claude Opus 5.5 --- ...xchange_direct_resolve_integration_test.go | 124 ++++++++++++++++++ 1 file changed, 124 insertions(+) create mode 100644 internal/api/handler_ri_exchange_direct_resolve_integration_test.go diff --git a/internal/api/handler_ri_exchange_direct_resolve_integration_test.go b/internal/api/handler_ri_exchange_direct_resolve_integration_test.go new file mode 100644 index 000000000..aa3eb866a --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_resolve_integration_test.go @@ -0,0 +1,124 @@ +//go:build integration + +package api + +import ( + "context" + "encoding/json" + "fmt" + "testing" + "time" + + "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/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// One AWS RI keeps the second execute's idempotency key apart from the first +// execute's, so the refusal and the later success come from the in-flight +// guard alone. +const directResolveSecondBody = `{ + "ri_ids": ["ri-0a1b2c3d"], + "targets": [{"offering_id": "` + auditOfferingA + `", "count": 1}], + "max_payment_due_usd": "500.00", + "region": "eu-central-1" +}` + +func directResolveHistoryRecord(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"} +} + +func listDirectRows(t *testing.T, h *Handler) riExchangeDirectListResponse { + t.Helper() + res, err := h.listRIExchangeDirectExecutions(auditListRequest()) + require.NoError(t, err) + // Round-trip through JSON, as a client sees it. + raw, err := json.Marshal(res) + require.NoError(t, err) + var out riExchangeDirectListResponse + require.NoError(t, json.Unmarshal(raw, &out)) + return out +} + +func auditListRequest() (context.Context, *events.LambdaFunctionURLRequest) { + return context.Background(), &events.LambdaFunctionURLRequest{Headers: map[string]string{"authorization": "Bearer tok"}} +} + +// #1836 end to end against a real database: a lost accept leaves an unknown +// row, the guard refuses the same RI, the operator lists and resolves the row +// through the HTTP handlers, and every path then proceeds. +func TestDirectExchangeResolve_EndToEnd(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequireMigratedDatabase(ctx, t, getMigrationsPath()) + store := config.NewPostgresStore(container.DB) + lostAccept := true + h, calls := directGuardHandler(t, store, func(context.Context, exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error) { + if lostAccept { + return "", nil, common.ErrOutcomeUnknown + } + return "riex-1", &exchange.ExchangeQuoteSummary{PaymentDueUSDStr: "40.250000", PaymentDueRaw: "40.25", CurrencyCode: "USD"}, nil + }) + + // 1. The accept outcome is lost. + _, err := h.executeExchange(auditRequest(ctx, awsAuditBody)) + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + require.Equal(t, 502, ce.code) + require.Equal(t, 1, *calls) + + // 2. The same RI is refused, with no further AWS call. + _, err = h.executeExchange(auditRequest(ctx, directResolveSecondBody)) + ce, ok = IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, 409, ce.code) + assert.Equal(t, 1, *calls, "a refused execute must not call AWS") + + // 3. The list shows the row, with no raw error. + rows := listDirectRows(t, h).Executions + require.Len(t, rows, 1) + row := rows[0] + assert.Equal(t, config.RIExchangeDirectStatusUnknown, row.Status) + assert.Equal(t, []string{"ri-0a1b2c3d", "ri-4e5f6a7b"}, row.SourceIDs) + + // 4. Resolve through the handler. The row's times are set relative to the + // database clock, and the operator's check is one minute and a bit after + // completion, read back from the database so no Go clock is involved. + _, err = container.DB.Exec(ctx, `UPDATE ri_exchange_direct_executions + SET created_at = now() - interval '2 hours', completed_at = now() - interval '1 hour' WHERE id = $1`, row.ID) + require.NoError(t, err) + var checkedAt time.Time + require.NoError(t, container.DB.QueryRow(ctx, + `SELECT completed_at + interval '2 minutes' FROM ri_exchange_direct_executions WHERE id = $1`, row.ID).Scan(&checkedAt)) + body := fmt.Sprintf(`{"outcome":"failed","expected_status":"unknown","reason":"checked at AWS, no exchange happened", + "acknowledge_unknown_outcome":true,"cloud_check":{"checked_region":"eu-central-1","checked_at":%q}}`, checkedAt.UTC().Format(time.RFC3339)) + res, err := h.resolveRIExchangeDirectExecution(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, Body: body}, row.ID) + require.NoError(t, err) + resolved, ok := res.(riExchangeDirectResolveResponse) + require.True(t, ok) + assert.Equal(t, config.RIExchangeDirectStatusFailed, resolved.ResolvedStatus) + assert.Equal(t, config.RIExchangeDirectStatusUnknown, resolved.Status, "the original status is kept") + assert.False(t, resolved.Replayed) + + // 5. The list no longer shows the row. + assert.Empty(t, listDirectRows(t, h).Executions) + + // 6. A direct execute of the RI proceeds, and so do AUTO and approve + // reservations of the formerly held RIs. + lostAccept = false + _, err = h.executeExchange(auditRequest(ctx, directResolveSecondBody)) + require.NoError(t, err, "a direct execute of a resolved RI proceeds") + assert.Equal(t, 2, *calls) + + _, err = store.ReserveRIExchange(ctx, directResolveHistoryRecord("ri-0a1b2c3d"), "1000", "1000") + require.NoError(t, err, "AUTO reservation of a resolved RI") + approving := directResolveHistoryRecord("ri-4e5f6a7b") + require.NoError(t, store.SaveRIExchangeRecord(ctx, approving)) + _, err = store.ReserveApprovedRIExchange(ctx, approving.ID, "5000", "1000") + require.NoError(t, err, "approval of a resolved RI") +} From 26166f0b1575f05730d420d603b9e0924af3bd42 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 8 Nov 2026 10:11:55 +0100 Subject: [PATCH 19/30] docs(runbooks): make the API the primary path for stuck direct exchanges List then resolve through the API; keep the SQL as a marked last resort that sets the five resolution columns under the store's age and checked_at rules. The runbook test now runs that SQL and pins its intervals to the Go constants. Drop the resolved known-issues entry. Refs #1836 Co-Authored-By: Claude Opus 5.5 --- ...xchange_direct_runbook_integration_test.go | 136 +++++++++++++----- known-issues.md | 6 - .../ri-exchange-direct-execution-settle.md | 135 ++++++++++++----- 3 files changed, 206 insertions(+), 71 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 0546f1491..4e47c3967 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 @@ -5,58 +5,128 @@ package config import ( "os" "regexp" + "strconv" "strings" "testing" + "time" "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' psql -// placeholders become positional parameters. -func runbookSettleSQL(t *testing.T) string { +// runbookBreakGlassSQLRaw reads the last-resort UPDATE from the runbook +// itself, so the operator's copy and the tested statement cannot drift. +func runbookBreakGlassSQLRaw(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])) + return string(m[1]) } -// #1829 runbook: the settle statement releases only rows it is safe to release. -func TestRIExchangeDirectRunbook_SettleStatement(t *testing.T) { +// runbookBreakGlassSQL turns the :'name' psql placeholders into positional +// parameters. +func runbookBreakGlassSQL(t *testing.T) string { + t.Helper() + return strings.NewReplacer( + ":'outcome'", "$1::text", ":'id'", "$2::uuid", ":'operator'", "$3::text", + ":'note'", "$4::text", ":'checked_at'", "$5::timestamptz", ":'expected_status'", "$6::text", + ).Replace(runbookBreakGlassSQLRaw(t)) +} + +// The intervals in the SQL are the store's constants, not copies that can drift. +func TestRIExchangeDirectRunbook_IntervalsMatchStore(t *testing.T) { + sql := runbookBreakGlassSQL(t) + minutes := func(re string) []time.Duration { + var out []time.Duration + for _, m := range regexp.MustCompile(re).FindAllStringSubmatch(sql, -1) { + n, err := strconv.Atoi(m[1]) + require.NoError(t, err) + out = append(out, time.Duration(n)*time.Minute) + } + return out + } + assert.Equal(t, []time.Duration{RIExchangeDirectResolveSubmittedAfter, RIExchangeDirectResolveSubmittedAfter}, + minutes(`interval '(\d+) minutes'`), "both submitted-age intervals equal the store constant") + assert.Equal(t, []time.Duration{RIExchangeDirectResolveUnknownCheckAfter}, + minutes(`interval '(\d+) minute'`), "the unknown check interval equals the store constant") +} + +// The note limit in the SQL is the store's constant. +func TestRIExchangeDirectRunbook_NoteLimitMatchesStore(t *testing.T) { + m := regexp.MustCompile(`octet_length\(:'note'\) <= (\d+)`).FindStringSubmatch(runbookBreakGlassSQLRaw(t)) + require.NotNil(t, m, "runbook SQL has no note length guard") + n, err := strconv.Atoi(m[1]) + require.NoError(t, err) + assert.Equal(t, RIExchangeDirectResolveNoteMax, n) +} + +// The last-resort statement resolves only rows the store would resolve, under +// the same age and checked_at rules, and a resolved row stops blocking. +func TestRIExchangeDirectRunbook_BreakGlassStatement(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) + sql := runbookBreakGlassSQL(t) + // Fixtures are relative to the database clock; the checked_at values are + // computed in SQL from the row's own times, never from Go time. + checkedAt := func(id, offset string) time.Time { + var at time.Time + require.NoError(t, env.poolA.QueryRow(env.ctx, ` + SELECT CASE WHEN status = 'submitted' THEN created_at + interval '10 minutes' ELSE completed_at + interval '1 minute' END + + ($2::text)::interval + FROM ri_exchange_direct_executions WHERE id = $1`, id, offset).Scan(&at)) + return at + } + agedFor := func(provider, status, ri, createdAgo, completedAgo string) string { + id := env.directRow(t, provider, status, ri) + _, err := env.poolA.Exec(env.ctx, ` + UPDATE ri_exchange_direct_executions + SET created_at = now() - ($2::text)::interval, + completed_at = CASE WHEN status = 'submitted' THEN NULL ELSE now() - ($3::text)::interval END + WHERE id = $1`, id, createdAgo, completedAgo) require.NoError(t, err) + return id } - settle := func(id string) int64 { - tag, err := env.poolA.Exec(env.ctx, sql, "failed", id, "checked at AWS") + aged := func(status, ri, createdAgo, completedAgo string) string { + return agedFor("aws", status, ri, createdAgo, completedAgo) + } + settleWith := func(id, outcome, expected, note string, at time.Time) int64 { + tag, err := env.poolA.Exec(env.ctx, sql, outcome, id, "operator-a", note, at, expected) require.NoError(t, err) return tag.RowsAffected() } + settle := func(id, outcome string, at time.Time) int64 { + var status string + require.NoError(t, env.poolA.QueryRow(env.ctx, "SELECT status FROM ri_exchange_direct_executions WHERE id = $1", id).Scan(&status)) + return settleWith(id, outcome, status, "checked at AWS", at) + } + + unknown := aged("unknown", "ri-g1836-rb-unknown", "2 hours", "1 hour") + assert.EqualValues(t, 0, settle(unknown, "failed", checkedAt(unknown, "-1 microsecond")), "unknown: checked_at just before completed_at + 1 minute") + assert.EqualValues(t, 0, settle(unknown, "failed", checkedAt(unknown, "10 hours")), "unknown: checked_at in the future") + require.ErrorIs(t, env.reserve(t, testAccountID(850000000001), "ri-g1836-rb-unknown"), ErrRIExchangeSourceInFlight, "an unresolved row keeps blocking") + assert.EqualValues(t, 0, settleWith(unknown, "failed", "submitted", "checked at AWS", checkedAt(unknown, "0")), "a status other than the one seen is not resolved") + assert.EqualValues(t, 0, settleWith(unknown, "failed", "unknown", strings.Repeat("x", RIExchangeDirectResolveNoteMax+1), checkedAt(unknown, "0")), "a note over the store limit is not stored") + assert.EqualValues(t, 1, settle(unknown, "failed", checkedAt(unknown, "0")), "unknown: exactly completed_at + 1 minute") + assert.EqualValues(t, 0, settle(unknown, "failed", checkedAt(unknown, "0")), "a resolved row is not resolved twice") + var status, resolved, by, note string + var userID *string + require.NoError(t, env.poolA.QueryRow(env.ctx, ` + SELECT status, resolved_status, resolved_by, resolution_note, resolved_by_user_id::text + FROM ri_exchange_direct_executions WHERE id = $1`, unknown).Scan(&status, &resolved, &by, ¬e, &userID)) + assert.Equal(t, []any{"unknown", "failed", "operator-a", "checked at AWS", (*string)(nil)}, []any{status, resolved, by, note, userID}) + require.NoError(t, env.reserve(t, testAccountID(850000000001), "ri-g1836-rb-unknown"), "a resolved row stops blocking the guard") + + old := aged("submitted", "ri-g1836-rb-old", "11 minutes", "0") + assert.EqualValues(t, 0, settle(old, "completed", checkedAt(old, "-1 microsecond")), "submitted: checked_at before created_at + 10 minutes") + assert.EqualValues(t, 1, settle(old, "completed", checkedAt(old, "0")), "submitted older than 10 minutes, checked at exactly the bound") + + young := aged("submitted", "ri-g1836-rb-young", "6 minutes", "0") + assert.EqualValues(t, 0, settle(young, "failed", checkedAt(young, "0")), "submitted inside the 10 minute window") - 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, "11 minutes") - assert.EqualValues(t, 1, settle(old), "a submitted row older than 10 minutes is settled") - - young := env.directRow(t, "aws", "submitted", "ri-g1829-rb-young") - age(young, "6 minutes") - assert.EqualValues(t, 0, settle(young), "a submitted row inside the 10 minute window 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") + // Old enough and checked late enough that only the provider test refuses it. + azure := agedFor("azure", "unknown", "ri-g1836-rb-azure", "2 hours", "1 hour") + assert.EqualValues(t, 0, settle(azure, "failed", checkedAt(azure, "0")), "Azure rows are untouched") + done := env.directRow(t, "aws", "completed", "ri-g1836-rb-done") + assert.EqualValues(t, 0, settle(done, "failed", time.Now().UTC().Add(-time.Minute)), "a completed row is final") } diff --git a/known-issues.md b/known-issues.md index dd115a3b1..b0fe11787 100644 --- a/known-issues.md +++ b/known-issues.md @@ -145,12 +145,6 @@ 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 diff --git a/runbooks/ri-exchange-direct-execution-settle.md b/runbooks/ri-exchange-direct-execution-settle.md index fb3acb1f2..ba0bbe9dc 100644 --- a/runbooks/ri-exchange-direct-execution-settle.md +++ b/runbooks/ri-exchange-direct-execution-settle.md @@ -12,57 +12,128 @@ because the exchange may already have happened at AWS. 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. +Resolving a row records what you found at AWS and releases its RIs. The row keeps its +original `status`; the resolution is stored beside it (`resolved_status`, `resolved_by`, +`resolved_at`, `resolution_note`), and rows with a resolution no longer block anything. ## Step 1: confirm the outcome at AWS Run `DescribeReservedInstances` (and check the target offering) for the source RIs -named in the refusal. Decide: +named in the refusal, in the row's region. Decide: -- no exchange happened: settle as `failed`; -- the exchange happened: settle as `completed`. +- no exchange happened: resolve as `failed`; +- the exchange happened: resolve as `completed`. -Do not guess. Marking a row `failed` releases the RIs for another exchange. +Do not guess. Resolving `failed` releases the RIs for another exchange. Note the time you +ran the check: the resolve asks for it (`checked_at`). -## Step 2: find the audit row id +## Step 2: list the stuck rows (API, primary path) -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'`; 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 ''`): +You need `execute:ri-exchange` on the row's account and region, as a signed-in user or a +user API key (the admin API key is refused). Placeholders: ``, ``, +``, ``. + +```sh +curl -s -H "Authorization: Bearer " \ + "https:///api/ri-exchange/direct-executions?status=unknown®ion=&ri_id=" +``` + +All query parameters are optional (`status` is `submitted` or `unknown`, default both; +`limit` 1 to 200; `cursor` from `next_cursor`). Each item has `id`, `status`, +`source_ids`, `created_at`, `error_summary`, `resolvable` and `resolvable_at`. Resolved +rows are not listed. A row outside your allowed accounts is not listed either. + +## Step 3: resolve the row (API, primary path) + +```sh +curl -s -X POST \ + -H "Authorization: Bearer " -H "X-CSRF-Token: " \ + -H "Content-Type: application/json" \ + "https:///api/ri-exchange/direct-executions//resolve" \ + -d '{ + "outcome": "failed", + "expected_status": "unknown", + "reason": "DescribeReservedInstances shows the RIs still active, no exchange", + "acknowledge_unknown_outcome": true, + "cloud_check": {"checked_region": "", "checked_at": ""} + }' +``` + +- `expected_status` is the status you saw in the list. If the row changed meanwhile you get 409. +- `outcome: "failed"` needs `acknowledge_unknown_outcome: true`. `completed` does not. +- `reason` is 10 to 300 characters, no control characters. +- `checked_region` must be the row's region. +- A repeat of the same request returns the stored result with `replayed: true`. +- 409 with "wait until" and a time means the row is too young; see "Why the age" below. +- 400 "checked_at must be at or after" and a time means your AWS check is too early; see + "Why checked_at has a lower bound" below. Check again at AWS, then retry with the new time. + +## Why the age + +A `submitted` row can only be resolved 10 minutes after it was created. The claim and +the AWS call share one deadline of 4 minutes measured from just before the row is +saved, so a `submitted` row younger than 10 minutes (about 4 minutes plus 6 minutes of +margin for clock skew and finalize) may still be in flight. Resolving it `failed` would +release the RI while AWS might still commit the accept. `created_at` is taken with +`clock_timestamp()` after the guard lock is acquired, so a lock wait does not age the +row. An `unknown` row has no age rule: its accept already returned. + +## Why checked_at has a lower bound + +Your AWS check only counts if it was made after the exchange could no longer change. +So `checked_at` must not be in the future, and must be at or after: + +- for a `submitted` row: `created_at` plus 10 minutes (the point where the process + that was running it is certainly finished); +- for an `unknown` row: `completed_at` plus 1 minute (the accept had returned, and the + 1 minute allows for EC2 `DescribeReservedInstances` being eventually consistent). + +A check made earlier proves nothing in either direction. The list shows `resolvable_at`, +which is that same earliest time, so a check at or after it is accepted. + +## LAST RESORT: settle the row in SQL + +Use this only when the API itself is unavailable. It bypasses the API's permission and +account checks and its audit log line, so a second person should review it. Run it only +after Step 1, in the application database, with the same rules as the API. + +Find the row id from the server log (search for "held by direct execute"), or by RI id +(set the psql variables first, for example +`psql -v ri=ri-0abc -v id= -v outcome=failed -v expected_status=unknown -v operator= -v checked_at='2025-01-01T12:00:00Z' -v note='checked at AWS'`): ```sql -SELECT id, status, created_at +SELECT id, status, created_at, completed_at FROM ri_exchange_direct_executions - WHERE provider = 'aws' AND status IN ('submitted', 'unknown') + WHERE provider = 'aws' AND status IN ('submitted', 'unknown') AND resolved_at IS NULL 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) +-- :'id' = the audit row id; :'expected_status' = the status the SELECT showed ('submitted' or 'unknown'); +-- :'outcome' = 'failed' (AWS shows no exchange) or 'completed' (AWS shows it happened); +-- :'operator' = your name; :'checked_at' = when you checked AWS; :'note' = what you found 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') + SET resolved_status = :'outcome', + resolved_by_user_id = NULL, + resolved_by = :'operator', + resolved_at = now(), + resolution_note = :'note' WHERE id = :'id' AND provider = 'aws' + AND resolved_at IS NULL + AND status = :'expected_status' AND status IN ('submitted', 'unknown') - AND (status = 'unknown' OR created_at < now() - interval '10 minutes'); --- Expect exactly 1 row updated. 0 rows means: wrong id, already settled, or a 'submitted' row younger --- than 10 minutes that may still be in flight. Do NOT widen the WHERE clause; wait and re-check. + AND octet_length(:'note') <= 1000 + AND ( (status = 'submitted' AND created_at <= now() - interval '10 minutes' + AND :'checked_at'::timestamptz >= created_at + interval '10 minutes') + OR (status = 'unknown' AND :'checked_at'::timestamptz >= completed_at + interval '1 minute')) + AND :'checked_at'::timestamptz <= now(); +-- Expect exactly 1 row updated. 0 rows means: wrong id, already resolved, a status that changed since +-- the SELECT, a note over 1000 bytes, a 'submitted' row younger than 10 minutes that may still be in +-- flight, or a checked_at that is too early or in the future. +-- Do NOT widen the WHERE clause; wait, check AWS again and re-run. ``` -- **Why the age check:** the claim and the AWS call share one deadline of 4 minutes - (`riExchangeExecutionTimeout`) measured from just before the row is saved, so a - `submitted` row younger than 10 minutes (about 4 minutes plus 6 minutes of margin for - clock skew and finalize) 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 10 minutes is past the execution window, claim included. -- **Why `error` is appended:** it keeps the original failure text. -- An `unknown` row is settled at any age: its accept already returned. +The status and `error` of the row are left as they were; the columns above are all five +that make up a resolution (the table rejects a partial one). From ad1be75d8b81bd1122a72a67e9e3ab32a56a059b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 14:02:10 +0100 Subject: [PATCH 20/30] test(api): pin the resolve response status enum to the store statuses A late finalize can move a resolved row to completed or failed, so the 200 schema must list every provider status. Refs #1836 Co-Authored-By: Claude Sonnet 5.5 --- ...ri_exchange_direct_resolve_openapi_test.go | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 internal/api/handler_ri_exchange_direct_resolve_openapi_test.go diff --git a/internal/api/handler_ri_exchange_direct_resolve_openapi_test.go b/internal/api/handler_ri_exchange_direct_resolve_openapi_test.go new file mode 100644 index 000000000..9b2682635 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_resolve_openapi_test.go @@ -0,0 +1,49 @@ +package api + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" +) + +// A late finalize can move a resolved row to completed or failed, so the +// resolve response must document every provider status. +func TestOpenAPIDirectResolveStatusEnumMatchesGoStatuses(t *testing.T) { + _, thisFile, _, _ := runtime.Caller(0) + data, err := os.ReadFile(filepath.Join(filepath.Dir(thisFile), "openapi.yaml")) + require.NoError(t, err) + type enumProp struct { + Enum []string `yaml:"enum"` + } + var doc struct { + Paths map[string]struct { + Post struct { + Responses map[string]struct { + Content struct { + JSON struct { + Schema struct { + Properties struct { + Status enumProp `yaml:"status"` + } `yaml:"properties"` + } `yaml:"schema"` + } `yaml:"application/json"` + } `yaml:"content"` + } `yaml:"responses"` + } `yaml:"post"` + } `yaml:"paths"` + } + require.NoError(t, yaml.Unmarshal(data, &doc)) + path, ok := doc.Paths["/api/ri-exchange/direct-executions/{id}/resolve"] + require.True(t, ok, "resolve path is documented") + require.ElementsMatch(t, []string{ + config.RIExchangeDirectStatusSubmitted, + config.RIExchangeDirectStatusUnknown, + config.RIExchangeDirectStatusCompleted, + config.RIExchangeDirectStatusFailed, + }, path.Post.Responses["200"].Content.JSON.Schema.Properties.Status.Enum) +} From b5b3d6ef3320f0af81a57d3eba457d55ff20140f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 14:15:49 +0100 Subject: [PATCH 21/30] style(api): fix the golangci-lint findings in the direct-exchange list code Name the validate result, stop shadowing err in the list handler, preallocate four slices in tests and apply De Morgan to the keyset test fake. No behavior change. Refs #1836 Co-Authored-By: Claude Sonnet 5.5 --- internal/api/handler_ri_exchange_direct_list.go | 2 +- internal/api/handler_ri_exchange_direct_list_test.go | 4 ++-- internal/config/store_postgres_ri_exchange_direct_list.go | 6 +++--- ...ore_postgres_ri_exchange_direct_list_integration_test.go | 4 ++-- ..._postgres_ri_exchange_direct_runbook_integration_test.go | 5 +++-- 5 files changed, 11 insertions(+), 10 deletions(-) diff --git a/internal/api/handler_ri_exchange_direct_list.go b/internal/api/handler_ri_exchange_direct_list.go index be016c514..ddb0d55b8 100644 --- a/internal/api/handler_ri_exchange_direct_list.go +++ b/internal/api/handler_ri_exchange_direct_list.go @@ -30,7 +30,7 @@ func (h *Handler) listRIExchangeDirectExecutions(ctx context.Context, req *event } filter.Allowed = allowed page, err := store.ListUnresolvedRIExchangeDirectExecutions(ctx, filter) - if err := mapRIExchangeResolveError(err); err != nil { + if err = mapRIExchangeResolveError(err); err != nil { return nil, err } now := time.Now().UTC() diff --git a/internal/api/handler_ri_exchange_direct_list_test.go b/internal/api/handler_ri_exchange_direct_list_test.go index b5b2459ca..94759d305 100644 --- a/internal/api/handler_ri_exchange_direct_list_test.go +++ b/internal/api/handler_ri_exchange_direct_list_test.go @@ -61,7 +61,7 @@ func (s *directListStore) ListUnresolvedRIExchangeDirectExecutions(_ context.Con if !allowed[pairKey(r.CloudAccountID, r.Region)] { continue } - if f.After != nil && !(r.CreatedAt.After(f.After.CreatedAt) || r.CreatedAt.Equal(f.After.CreatedAt) && r.ID > f.After.ID) { + if f.After != nil && !r.CreatedAt.After(f.After.CreatedAt) && (!r.CreatedAt.Equal(f.After.CreatedAt) || r.ID <= f.After.ID) { continue } rows = append(rows, r) @@ -115,7 +115,7 @@ func dlIDs(t *testing.T, out any) []string { } func regionGrant(regions ...string) []auth.Permission { - var perms []auth.Permission + perms := make([]auth.Permission, 0, len(regions)) for _, r := range regions { perms = append(perms, auth.Permission{Action: auth.ActionExecute, Resource: "ri-exchange", Constraints: &auth.PermissionConstraints{Regions: []string{r}}}) diff --git a/internal/config/store_postgres_ri_exchange_direct_list.go b/internal/config/store_postgres_ri_exchange_direct_list.go index ed18ddc99..4f11d67c0 100644 --- a/internal/config/store_postgres_ri_exchange_direct_list.go +++ b/internal/config/store_postgres_ri_exchange_direct_list.go @@ -92,15 +92,15 @@ func (s *PostgresStore) ListUnresolvedRIExchangeDirectExecutions(ctx context.Con // validateRIExchangeDirectListFilter applies the limit default and cap and // checks the status filter. -func validateRIExchangeDirectListFilter(f RIExchangeDirectListFilter) (int, []string, error) { - limit := f.Limit +func validateRIExchangeDirectListFilter(f RIExchangeDirectListFilter) (limit int, statuses []string, err error) { + limit = f.Limit if limit <= 0 { limit = RIExchangeDirectListDefaultLimit } if limit > RIExchangeDirectListMaxLimit { return 0, nil, fmt.Errorf("%w: limit must be at most %d", ErrRIExchangeResolveInvalid, RIExchangeDirectListMaxLimit) } - statuses := f.Statuses + statuses = f.Statuses if len(statuses) == 0 { statuses = []string{RIExchangeDirectStatusSubmitted, RIExchangeDirectStatusUnknown} } diff --git a/internal/config/store_postgres_ri_exchange_direct_list_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_list_integration_test.go index 9377b547f..399701923 100644 --- a/internal/config/store_postgres_ri_exchange_direct_list_integration_test.go +++ b/internal/config/store_postgres_ri_exchange_direct_list_integration_test.go @@ -162,7 +162,7 @@ func TestPostgresStoreDB_RIExchangeDirectList_KeysetPagination(t *testing.T) { env := newRIExchangeIntegrationEnv(t) acc := strPtr("acc-1") base := time.Now().UTC().Truncate(time.Microsecond).Add(-time.Hour) - var all []string + all := make([]string, 0, 7) for _, at := range []time.Time{base, base, base, base, base.Add(time.Second), base.Add(time.Second), base.Add(2 * time.Second)} { all = append(all, env.insertDirect(t, directSpec{account: acc, createdAt: at})) } @@ -170,9 +170,9 @@ func TestPostgresStoreDB_RIExchangeDirectList_KeysetPagination(t *testing.T) { at time.Time id string } - var keys []key full, err := env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, RIExchangeDirectListFilter{Allowed: allow(acc, "eu-west-1")}) require.NoError(t, err) + keys := make([]key, 0, len(full.Rows)) for _, r := range full.Rows { keys = append(keys, key{r.CreatedAt, r.ID}) } 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 4e47c3967..1d1685dc7 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 @@ -39,8 +39,9 @@ func runbookBreakGlassSQL(t *testing.T) string { func TestRIExchangeDirectRunbook_IntervalsMatchStore(t *testing.T) { sql := runbookBreakGlassSQL(t) minutes := func(re string) []time.Duration { - var out []time.Duration - for _, m := range regexp.MustCompile(re).FindAllStringSubmatch(sql, -1) { + matches := regexp.MustCompile(re).FindAllStringSubmatch(sql, -1) + out := make([]time.Duration, 0, len(matches)) + for _, m := range matches { n, err := strconv.Atoi(m[1]) require.NoError(t, err) out = append(out, time.Duration(n)*time.Minute) From a4a0e642f3450f77cbf2c6f4778ec14a9645a9ee Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 15:22:26 +0100 Subject: [PATCH 22/30] style(api): map the list store error like the resolve handler Use the mapped-error pattern the resolve handlers use, which golangci-lint's sloppyReassign accepts. No behavior change. Refs #1836 Co-Authored-By: Claude Sonnet 5.5 --- internal/api/handler_ri_exchange_direct_list.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/api/handler_ri_exchange_direct_list.go b/internal/api/handler_ri_exchange_direct_list.go index ddb0d55b8..876503d8b 100644 --- a/internal/api/handler_ri_exchange_direct_list.go +++ b/internal/api/handler_ri_exchange_direct_list.go @@ -30,8 +30,8 @@ func (h *Handler) listRIExchangeDirectExecutions(ctx context.Context, req *event } filter.Allowed = allowed page, err := store.ListUnresolvedRIExchangeDirectExecutions(ctx, filter) - if err = mapRIExchangeResolveError(err); err != nil { - return nil, err + if mapped := mapRIExchangeResolveError(err); mapped != nil { + return nil, mapped } now := time.Now().UTC() resp := riExchangeDirectListResponse{Executions: make([]riExchangeDirectListItem, 0, len(page.Rows))} From 4dc88765bdce411e4fe8680a16364be19aca0619 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 15:23:16 +0100 Subject: [PATCH 23/30] fix(api): keep sub-second checked_at in the resolve note and show the account The note stored checked_at to the second, so two different checks a fraction of a second apart replayed instead of conflicting. Keep the full precision. List items now carry cloud_account_id (null for an unattributed row), the openapi reason text states the 1000-byte stored-note limit, and a refusal fixture gets a wider age margin. Refs #1836 Co-Authored-By: Claude Sonnet 5.5 --- ...andler_ri_exchange_direct_list_item_test.go | 15 ++++++++++++++- .../api/handler_ri_exchange_direct_resolve.go | 3 ++- ...ndler_ri_exchange_direct_resolve_request.go | 2 +- .../handler_ri_exchange_direct_resolve_test.go | 16 ++++++++++++++++ internal/api/openapi.yaml | 18 ++++++++++++++---- ...exchange_direct_resolve_integration_test.go | 4 ++-- 6 files changed, 49 insertions(+), 9 deletions(-) diff --git a/internal/api/handler_ri_exchange_direct_list_item_test.go b/internal/api/handler_ri_exchange_direct_list_item_test.go index e9cc6896a..c23be4485 100644 --- a/internal/api/handler_ri_exchange_direct_list_item_test.go +++ b/internal/api/handler_ri_exchange_direct_list_item_test.go @@ -67,7 +67,7 @@ func TestRIExchangeDirectListItem_KeysAreAllowlisted(t *testing.T) { var items []map[string]any require.NoError(t, json.Unmarshal(top["executions"], &items)) require.Len(t, items, 1) - assert.ElementsMatch(t, []string{"id", "status", "provider", "region", "source_ids", "targets", "max_payment_due", + assert.ElementsMatch(t, []string{"id", "status", "provider", "region", "cloud_account_id", "source_ids", "targets", "max_payment_due", "max_payment_currency", "created_at", "executed_by", "error_summary", "resolvable", "resolvable_at"}, keysOf(items[0])) assert.Equal(t, purchase.SanitizeStoredError(row.Error), items[0]["error_summary"]) @@ -77,6 +77,19 @@ func TestRIExchangeDirectListItem_KeysAreAllowlisted(t *testing.T) { assert.Equal(t, "aws", items[0]["provider"]) } +func TestRIExchangeDirectListItem_CloudAccountID(t *testing.T) { + attributed := dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1) + unattributed := dlRow(dlID2, nil, "eu-west-1", "unknown", 1) + + raw, err := json.Marshal(newRIExchangeDirectListItem(&attributed, time.Now())) + require.NoError(t, err) + assert.Contains(t, string(raw), `"cloud_account_id":"`+dlAcctA+`"`) + + raw, err = json.Marshal(newRIExchangeDirectListItem(&unattributed, time.Now())) + require.NoError(t, err) + assert.Contains(t, string(raw), `"cloud_account_id":null`) +} + func TestRIExchangeDirectListItem_ResolvableHint(t *testing.T) { now := time.Now().UTC() at := func(r config.RIExchangeDirectRow, created time.Time) config.RIExchangeDirectRow { diff --git a/internal/api/handler_ri_exchange_direct_resolve.go b/internal/api/handler_ri_exchange_direct_resolve.go index e34afa2c1..054fb1d7c 100644 --- a/internal/api/handler_ri_exchange_direct_resolve.go +++ b/internal/api/handler_ri_exchange_direct_resolve.go @@ -28,6 +28,7 @@ type riExchangeDirectListItem struct { Status string `json:"status"` Provider string `json:"provider"` Region string `json:"region"` + CloudAccountID *string `json:"cloud_account_id"` SourceIDs []string `json:"source_ids"` Targets json.RawMessage `json:"targets"` MaxPaymentDue string `json:"max_payment_due"` @@ -118,7 +119,7 @@ func newRIExchangeDirectListItem(r *config.RIExchangeDirectRow, now time.Time) r at := resolvableRIExchangeDirectAt(r) return riExchangeDirectListItem{ ID: r.ID, Status: r.Status, Provider: string(common.ProviderAWS), Region: r.Region, - SourceIDs: r.SourceIDs, Targets: r.Targets, + CloudAccountID: r.CloudAccountID, SourceIDs: r.SourceIDs, Targets: r.Targets, MaxPaymentDue: r.MaxPaymentDue, MaxPaymentCurrency: r.MaxPaymentCurrency, CreatedAt: r.CreatedAt.UTC(), ExecutedBy: r.ExecutedBy, ErrorSummary: purchase.SanitizeStoredError(r.Error), diff --git a/internal/api/handler_ri_exchange_direct_resolve_request.go b/internal/api/handler_ri_exchange_direct_resolve_request.go index d87e9e4d4..90e68429a 100644 --- a/internal/api/handler_ri_exchange_direct_resolve_request.go +++ b/internal/api/handler_ri_exchange_direct_resolve_request.go @@ -94,7 +94,7 @@ func (b riExchangeDirectResolveRequest) checkedAt(rowRegion string) (time.Time, // order and quoted, so a repeat of the same request produces the same string // and a different one cannot. func (b riExchangeDirectResolveRequest) note(checkedAt time.Time) string { - s := fmt.Sprintf("operator:%s: region=%q checked_at=%s", b.Outcome, b.CloudCheck.Region, checkedAt.Format(time.RFC3339)) + s := fmt.Sprintf("operator:%s: region=%q checked_at=%s", b.Outcome, b.CloudCheck.Region, checkedAt.Format(time.RFC3339Nano)) if b.Outcome == config.RIExchangeDirectStatusFailed { s += " ack=true" } diff --git a/internal/api/handler_ri_exchange_direct_resolve_test.go b/internal/api/handler_ri_exchange_direct_resolve_test.go index 396b73c72..40b275f5b 100644 --- a/internal/api/handler_ri_exchange_direct_resolve_test.go +++ b/internal/api/handler_ri_exchange_direct_resolve_test.go @@ -132,6 +132,22 @@ func TestResolveRIExchangeDirect_PassesOperatorDecisionToStore(t *testing.T) { assert.Equal(t, `operator:completed: region="eu-west-1" checked_at=2026-10-19T08:00:00Z reason="source RIs still active in the EC2 console"`, completed.got.Note) } +// The store replays only a byte-identical note and refuses any other as a +// conflict, so two checks a fraction of a second apart must not share a note. +func TestResolveRIExchangeDirect_SubSecondCheckedAtChangesNote(t *testing.T) { + body := func(at string) string { + return fmt.Sprintf(`{"outcome":"completed","expected_status":"unknown","reason":"source RIs still active in the EC2 console",`+ + `"cloud_check":{"checked_region":%q,"checked_at":%q}}`, drRegion, at) + } + first, second := drStore(), drStore() + _, err := drCaller{perms: riExecute}.call(first, dlID1, body("2026-10-19T08:00:00.1Z")) + require.NoError(t, err) + _, err = drCaller{perms: riExecute}.call(second, dlID1, body("2026-10-19T08:00:00.2Z")) + require.NoError(t, err) + assert.NotEqual(t, first.got.Note, second.got.Note, "equal notes would replay instead of conflicting") + assert.Contains(t, first.got.Note, "checked_at=2026-10-19T08:00:00.1Z") +} + func TestResolveRIExchangeDirect_ReplayIsReportedAndNotLogged(t *testing.T) { logs := captureLogs(t) store := drStore() diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 30131294a..d473505ba 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2949,7 +2949,7 @@ paths: type: array items: type: object - required: [id, status, provider, region, source_ids, targets, max_payment_due, max_payment_currency, created_at, executed_by, error_summary, resolvable, resolvable_at] + required: [id, status, provider, region, cloud_account_id, source_ids, targets, max_payment_due, max_payment_currency, created_at, executed_by, error_summary, resolvable, resolvable_at] properties: id: type: string @@ -2962,6 +2962,10 @@ paths: enum: [aws] region: type: string + cloud_account_id: + type: string + nullable: true + description: The registered cloud account of the exchange, null when it mapped to none. source_ids: type: array items: @@ -3049,7 +3053,11 @@ paths: type: string minLength: 10 maxLength: 300 - description: Free text, trimmed, without control characters. + description: > + Free text, trimmed, without control characters. At most + 300 characters, and the stored note at most 1000 bytes, so + non-ASCII text needs fewer characters. Past that the API + answers 400 "reason too long once encoded; shorten it". acknowledge_unknown_outcome: type: boolean description: Must be true when outcome is failed. @@ -3099,8 +3107,10 @@ paths: '400': description: > Invalid id or body: body over 4096 bytes, unknown fields, a - missing or invalid field, a region other than the row's, or a - `checked_at` outside the allowed window. + missing or invalid field, a reason over 300 characters or whose + stored note exceeds 1000 bytes ("reason too long once encoded; + shorten it"), a region other than the row's, or a `checked_at` + outside the allowed window. content: application/json: schema: diff --git a/internal/config/store_postgres_ri_exchange_direct_resolve_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_resolve_integration_test.go index 8f0c1f953..e5e4cffdc 100644 --- a/internal/config/store_postgres_ri_exchange_direct_resolve_integration_test.go +++ b/internal/config/store_postgres_ri_exchange_direct_resolve_integration_test.go @@ -57,13 +57,13 @@ func (e *riExchangeIntegrationEnv) resolve(t *testing.T, req RIExchangeDirectRes // recorded, and the guard stops blocking. func TestPostgresStoreDB_ResolveRIExchangeDirectExecution_SubmittedAgeBoundary(t *testing.T) { env := newRIExchangeIntegrationEnv(t) - young := env.directAged(t, "submitted", "9 minutes 59 seconds", "0") + young := env.directAged(t, "submitted", "9 minutes", "0") old := env.directAged(t, "submitted", "10 minutes 1 second", "0") _, err := env.resolve(t, directResolveReq(young, "failed", "submitted", time.Now().UTC())) require.ErrorIs(t, err, ErrRIExchangeResolveConflict) assert.Contains(t, err.Error(), "wait until") - require.ErrorIs(t, env.reserve(t, testAccountID(836100000001), "ri-g1836-submitted-9 minutes 59 seconds"), ErrRIExchangeSourceInFlight) + require.ErrorIs(t, env.reserve(t, testAccountID(836100000001), "ri-g1836-submitted-9 minutes"), ErrRIExchangeSourceInFlight) times := env.directTimes(t, old) uid := "7d444840-9dc0-11d1-b245-5ffdce74fad2" From c66dc2ae1d66ac63f4c89b45e771020c25bb96ba Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 15:23:57 +0100 Subject: [PATCH 24/30] docs(runbooks): keep checked_at and reject empty fields in the break-glass SQL The last-resort statement dropped the AWS check time from the stored note, let the byte limit skip the composed note and accepted an empty operator or note. Compose the note with checked_at, apply the limit to the whole note, require both fields, and say that a row with an empty region needs this path. Refs #1836 Co-Authored-By: Claude Sonnet 5.5 --- ...xchange_direct_runbook_integration_test.go | 20 +++++++++++-------- .../ri-exchange-direct-execution-settle.md | 15 +++++++++----- 2 files changed, 22 insertions(+), 13 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 1d1685dc7..0c62bd21a 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 @@ -56,7 +56,7 @@ func TestRIExchangeDirectRunbook_IntervalsMatchStore(t *testing.T) { // The note limit in the SQL is the store's constant. func TestRIExchangeDirectRunbook_NoteLimitMatchesStore(t *testing.T) { - m := regexp.MustCompile(`octet_length\(:'note'\) <= (\d+)`).FindStringSubmatch(runbookBreakGlassSQLRaw(t)) + m := regexp.MustCompile(`octet_length\(v\.n\) <= (\d+)`).FindStringSubmatch(runbookBreakGlassSQLRaw(t)) require.NotNil(t, m, "runbook SQL has no note length guard") n, err := strconv.Atoi(m[1]) require.NoError(t, err) @@ -91,31 +91,35 @@ func TestRIExchangeDirectRunbook_BreakGlassStatement(t *testing.T) { aged := func(status, ri, createdAgo, completedAgo string) string { return agedFor("aws", status, ri, createdAgo, completedAgo) } - settleWith := func(id, outcome, expected, note string, at time.Time) int64 { - tag, err := env.poolA.Exec(env.ctx, sql, outcome, id, "operator-a", note, at, expected) + settleWith := func(id, outcome, expected, operator, note string, at time.Time) int64 { + tag, err := env.poolA.Exec(env.ctx, sql, outcome, id, operator, note, at, expected) require.NoError(t, err) return tag.RowsAffected() } settle := func(id, outcome string, at time.Time) int64 { var status string require.NoError(t, env.poolA.QueryRow(env.ctx, "SELECT status FROM ri_exchange_direct_executions WHERE id = $1", id).Scan(&status)) - return settleWith(id, outcome, status, "checked at AWS", at) + return settleWith(id, outcome, status, "operator-a", "checked at AWS", at) } unknown := aged("unknown", "ri-g1836-rb-unknown", "2 hours", "1 hour") assert.EqualValues(t, 0, settle(unknown, "failed", checkedAt(unknown, "-1 microsecond")), "unknown: checked_at just before completed_at + 1 minute") assert.EqualValues(t, 0, settle(unknown, "failed", checkedAt(unknown, "10 hours")), "unknown: checked_at in the future") require.ErrorIs(t, env.reserve(t, testAccountID(850000000001), "ri-g1836-rb-unknown"), ErrRIExchangeSourceInFlight, "an unresolved row keeps blocking") - assert.EqualValues(t, 0, settleWith(unknown, "failed", "submitted", "checked at AWS", checkedAt(unknown, "0")), "a status other than the one seen is not resolved") - assert.EqualValues(t, 0, settleWith(unknown, "failed", "unknown", strings.Repeat("x", RIExchangeDirectResolveNoteMax+1), checkedAt(unknown, "0")), "a note over the store limit is not stored") - assert.EqualValues(t, 1, settle(unknown, "failed", checkedAt(unknown, "0")), "unknown: exactly completed_at + 1 minute") + assert.EqualValues(t, 0, settleWith(unknown, "failed", "submitted", "operator-a", "checked at AWS", checkedAt(unknown, "0")), "a status other than the one seen is not resolved") + assert.EqualValues(t, 0, settleWith(unknown, "failed", "unknown", "operator-a", strings.Repeat("x", RIExchangeDirectResolveNoteMax+1), checkedAt(unknown, "0")), "a note over the store limit is not stored") + assert.EqualValues(t, 0, settleWith(unknown, "failed", "unknown", "operator-a", strings.Repeat("x", RIExchangeDirectResolveNoteMax), checkedAt(unknown, "0")), "a note that overflows only with the checked_at prefix is not stored") + assert.EqualValues(t, 0, settleWith(unknown, "failed", "unknown", "operator-a", "", checkedAt(unknown, "0")), "an empty note is not stored") + assert.EqualValues(t, 0, settleWith(unknown, "failed", "unknown", " ", "checked at AWS", checkedAt(unknown, "0")), "an empty operator is not recorded") + bound := checkedAt(unknown, "0") + assert.EqualValues(t, 1, settle(unknown, "failed", bound), "unknown: exactly completed_at + 1 minute") assert.EqualValues(t, 0, settle(unknown, "failed", checkedAt(unknown, "0")), "a resolved row is not resolved twice") var status, resolved, by, note string var userID *string require.NoError(t, env.poolA.QueryRow(env.ctx, ` SELECT status, resolved_status, resolved_by, resolution_note, resolved_by_user_id::text FROM ri_exchange_direct_executions WHERE id = $1`, unknown).Scan(&status, &resolved, &by, ¬e, &userID)) - assert.Equal(t, []any{"unknown", "failed", "operator-a", "checked at AWS", (*string)(nil)}, []any{status, resolved, by, note, userID}) + assert.Equal(t, []any{"unknown", "failed", "operator-a", "break-glass checked_at=" + bound.UTC().Format("2006-01-02T15:04:05.000000Z") + " note=checked at AWS", (*string)(nil)}, []any{status, resolved, by, note, userID}) require.NoError(t, env.reserve(t, testAccountID(850000000001), "ri-g1836-rb-unknown"), "a resolved row stops blocking the guard") old := aged("submitted", "ri-g1836-rb-old", "11 minutes", "0") diff --git a/runbooks/ri-exchange-direct-execution-settle.md b/runbooks/ri-exchange-direct-execution-settle.md index ba0bbe9dc..c37a04694 100644 --- a/runbooks/ri-exchange-direct-execution-settle.md +++ b/runbooks/ri-exchange-direct-execution-settle.md @@ -61,8 +61,10 @@ curl -s -X POST \ - `expected_status` is the status you saw in the list. If the row changed meanwhile you get 409. - `outcome: "failed"` needs `acknowledge_unknown_outcome: true`. `completed` does not. -- `reason` is 10 to 300 characters, no control characters. -- `checked_region` must be the row's region. +- `reason` is 10 to 300 characters, no control characters, and the stored note must fit 1000 bytes. + Non-ASCII text needs fewer characters; the API answers 400 "reason too long once encoded; shorten it". +- `checked_region` must be the row's region. A row with an empty region cannot be resolved through the + API (400 on region); use the break-glass statement below. - A repeat of the same request returns the stored result with `replayed: true`. - 409 with "wait until" and a time means the row is too young; see "Why the age" below. - 400 "checked_at must be at or after" and a time means your AWS check is too early; see @@ -118,19 +120,22 @@ UPDATE ri_exchange_direct_executions resolved_by_user_id = NULL, resolved_by = :'operator', resolved_at = now(), - resolution_note = :'note' + resolution_note = v.n + FROM (SELECT 'break-glass checked_at=' || to_char(:'checked_at'::timestamptz AT TIME ZONE 'UTC', 'YYYY-MM-DD"T"HH24:MI:SS.US"Z"') + || ' note=' || :'note' AS n) v WHERE id = :'id' AND provider = 'aws' AND resolved_at IS NULL AND status = :'expected_status' AND status IN ('submitted', 'unknown') - AND octet_length(:'note') <= 1000 + AND octet_length(v.n) <= 1000 + AND btrim(:'operator') <> '' AND btrim(:'note') <> '' AND ( (status = 'submitted' AND created_at <= now() - interval '10 minutes' AND :'checked_at'::timestamptz >= created_at + interval '10 minutes') OR (status = 'unknown' AND :'checked_at'::timestamptz >= completed_at + interval '1 minute')) AND :'checked_at'::timestamptz <= now(); -- Expect exactly 1 row updated. 0 rows means: wrong id, already resolved, a status that changed since --- the SELECT, a note over 1000 bytes, a 'submitted' row younger than 10 minutes that may still be in +-- the SELECT, an empty operator or note, a stored note (with its prefix) over 1000 bytes, a 'submitted' row younger than 10 minutes that may still be in -- flight, or a checked_at that is too early or in the future. -- Do NOT widen the WHERE clause; wait, check AWS again and re-run. ``` From 60a362e15a9f1942d18764a768613138d57ea307 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 15:36:21 +0100 Subject: [PATCH 25/30] fix(config): keep exchanges working when the app runs ahead of migration 163 The source-RI guard and Finalize read the resolution columns, so an app deployed before migration 163 failed every reservation and left Azure direct rows submitted. Gate both on the schema version like the settlement columns, and make list, get and resolve answer a 503 until the migration is applied. Refs #1836 Co-Authored-By: Claude Sonnet 5.5 --- ...handler_ri_exchange_direct_resolve_test.go | 1 + internal/api/handler_ri_exchange_resolve.go | 2 + internal/config/ri_exchange_direct_resolve.go | 4 + internal/config/store_postgres.go | 9 +- .../config/store_postgres_pgxmock_test.go | 1 + .../store_postgres_ri_exchange_direct.go | 36 +++++- .../store_postgres_ri_exchange_direct_get.go | 6 + .../store_postgres_ri_exchange_direct_list.go | 34 ++++-- ...ore_postgres_ri_exchange_direct_resolve.go | 3 + ...exchange_direct_schema_integration_test.go | 41 +++++++ ...postgres_ri_exchange_direct_schema_test.go | 115 ++++++++++++++++++ .../store_postgres_ri_exchange_direct_test.go | 3 + .../config/store_postgres_schema_version.go | 30 +++++ .../store_postgres_schema_version_test.go | 25 ++++ 14 files changed, 293 insertions(+), 17 deletions(-) create mode 100644 internal/config/store_postgres_ri_exchange_direct_schema_integration_test.go create mode 100644 internal/config/store_postgres_ri_exchange_direct_schema_test.go diff --git a/internal/api/handler_ri_exchange_direct_resolve_test.go b/internal/api/handler_ri_exchange_direct_resolve_test.go index 40b275f5b..d2aac9b3e 100644 --- a/internal/api/handler_ri_exchange_direct_resolve_test.go +++ b/internal/api/handler_ri_exchange_direct_resolve_test.go @@ -166,6 +166,7 @@ func TestResolveRIExchangeDirect_MapsStoreOutcomes(t *testing.T) { want int }{ {"missing at resolve time", config.ErrRIExchangeResolveNotFound, 404}, + {"migration not applied", config.ErrRIExchangeDirectResolveNotReady, 503}, {"checked_at outside the window", fmt.Errorf("%w: checked_at is in the future", config.ErrRIExchangeResolveInvalid), 400}, {"already settled", fmt.Errorf("%w: the exchange is already completed", config.ErrRIExchangeResolveConflict), 409}, {"may still be in flight", fmt.Errorf("%w: the exchange may still be in flight; wait until 2026-11-08T09:10:00Z", config.ErrRIExchangeResolveConflict), 409}, diff --git a/internal/api/handler_ri_exchange_resolve.go b/internal/api/handler_ri_exchange_resolve.go index 65db477ab..e226906d4 100644 --- a/internal/api/handler_ri_exchange_resolve.go +++ b/internal/api/handler_ri_exchange_resolve.go @@ -192,6 +192,8 @@ func mapRIExchangeResolveError(err error) error { return NewClientError(400, err.Error()) case errors.Is(err, config.ErrRIExchangeResolveConflict): return NewClientError(409, err.Error()) + case errors.Is(err, config.ErrRIExchangeDirectResolveNotReady): + return NewClientError(503, err.Error()) } return err } diff --git a/internal/config/ri_exchange_direct_resolve.go b/internal/config/ri_exchange_direct_resolve.go index 7c03bfc93..660cb5700 100644 --- a/internal/config/ri_exchange_direct_resolve.go +++ b/internal/config/ri_exchange_direct_resolve.go @@ -2,6 +2,7 @@ package config import ( "context" + "errors" "time" ) @@ -47,6 +48,9 @@ type RIExchangeDirectResolveResult struct { Replayed bool } +// ErrRIExchangeDirectResolveNotReady: the resolution migration is not applied yet. +var ErrRIExchangeDirectResolveNotReady = errors.New("resolving direct RI exchanges is not available until the database migration is applied") + // RIExchangeDirectResolveStore is the store surface of the direct-row list and // resolve endpoints. type RIExchangeDirectResolveStore interface { diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 4a8d85604..435e1921f 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -3499,6 +3499,13 @@ func refuseRIExchangeSourceInFlightDirect(ctx context.Context, tx pgx.Tx, source if len(normalised) == 0 { return nil } + resolvedFilter := " AND resolved_at IS NULL" + if ready, err := directResolveSchemaReady(ctx, tx); err != nil { + logging.Errorf("RI exchange reservation: schema version check failed: %v", err) + return errors.New("failed to check RI exchange source RIs held by a direct execute") + } else if !ready { + resolvedFilter = "" + } var blocker string var shared []string err := tx.QueryRow(ctx, ` @@ -3506,7 +3513,7 @@ func refuseRIExchangeSourceInFlightDirect(ctx context.Context, tx pgx.Tx, source 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') AND resolved_at IS NULL + WHERE provider = 'aws' AND status IN ('submitted', 'unknown')`+resolvedFilter+` ) d WHERE ids && $1::text[] ORDER BY created_at, id diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index fa378c975..f45cc1a29 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -3586,6 +3586,7 @@ func TestPGXMock_ReserveApprovedRIExchange_ReservesCeilingExcludingOwnRow(t *tes 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). + expectDirectResolveVersion(mock, directResolveMigrationVersion) 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"). diff --git a/internal/config/store_postgres_ri_exchange_direct.go b/internal/config/store_postgres_ri_exchange_direct.go index 8c2df30bf..e40c5b38a 100644 --- a/internal/config/store_postgres_ri_exchange_direct.go +++ b/internal/config/store_postgres_ri_exchange_direct.go @@ -104,6 +104,12 @@ func (s *PostgresStore) FinalizeRIExchangeDirectExecution(ctx context.Context, i if err := outcome.Validate(); err != nil { return err } + ready, err := directResolveSchemaReady(ctx, s.db) + if err != nil { + // The provider outcome must not be lost over a version read: the plain UPDATE is right on either schema. + logging.Errorf("ri-exchange direct execute %s: schema version unreadable, finalizing without the resolution check: %v", id, err) + ready = false + } query := ` UPDATE ri_exchange_direct_executions SET status = $2, @@ -114,11 +120,8 @@ func (s *PostgresStore) FinalizeRIExchangeDirectExecution(ctx context.Context, i error = $7, completed_at = now() WHERE id = $1 - AND status = 'submitted' - RETURNING resolved_status - ` - var resolved *string - err := s.db.QueryRow(ctx, query, + AND status = 'submitted'` + args := []any{ id, outcome.Status, nullStringFromString(outcome.ProviderReference), @@ -126,7 +129,28 @@ func (s *PostgresStore) FinalizeRIExchangeDirectExecution(ctx context.Context, i nullStringFromString(outcome.NetPayableCurrency), nullStringFromString(outcome.ProviderStatus), nullStringFromString(outcome.Error), - ).Scan(&resolved) + } + if !ready { + // No resolved_status column yet (or the schema version is unreadable): nothing to check against. + return s.finalizeDirectPlain(ctx, id, query, args) + } + return s.finalizeDirectChecked(ctx, id, query, args, outcome) +} + +func (s *PostgresStore) finalizeDirectPlain(ctx context.Context, id, query string, args []any) error { + tag, err := s.db.Exec(ctx, query, args...) + if err != nil { + return fmt.Errorf("failed to finalize ri exchange direct execution %s: %w", id, err) + } + if tag.RowsAffected() == 0 { + return fmt.Errorf("finalize ri exchange direct execution %s: %w", id, ErrRIExchangeDirectExecutionNotSubmitted) + } + return nil +} + +func (s *PostgresStore) finalizeDirectChecked(ctx context.Context, id, query string, args []any, outcome RIExchangeDirectOutcome) error { + var resolved *string + err := s.db.QueryRow(ctx, query+"\n\t\tRETURNING resolved_status", args...).Scan(&resolved) if errors.Is(err, pgx.ErrNoRows) { return fmt.Errorf("finalize ri exchange direct execution %s: %w", id, ErrRIExchangeDirectExecutionNotSubmitted) } diff --git a/internal/config/store_postgres_ri_exchange_direct_get.go b/internal/config/store_postgres_ri_exchange_direct_get.go index dcd7bebd0..1c54cd2da 100644 --- a/internal/config/store_postgres_ri_exchange_direct_get.go +++ b/internal/config/store_postgres_ri_exchange_direct_get.go @@ -13,6 +13,9 @@ import ( // authorization service about each pair once. The unresolved partial index // serves it. func (s *PostgresStore) ListUnresolvedRIExchangeDirectAccountRegions(ctx context.Context) ([]RIExchangeDirectAccountRegion, error) { + if err := requireDirectResolveSchema(ctx, s.db); err != nil { + return nil, err + } rows, err := s.db.Query(ctx, ` SELECT DISTINCT cloud_account_id, COALESCE(region, '') FROM ri_exchange_direct_executions @@ -36,6 +39,9 @@ func (s *PostgresStore) ListUnresolvedRIExchangeDirectAccountRegions(ctx context // GetRIExchangeDirectExecution returns one AWS direct row of any status, or // ErrRIExchangeResolveNotFound for a missing or non-AWS row. func (s *PostgresStore) GetRIExchangeDirectExecution(ctx context.Context, id string) (*RIExchangeDirectRow, error) { + if err := requireDirectResolveSchema(ctx, s.db); err != nil { + return nil, err + } r, err := scanRIExchangeDirectRow(s.db.QueryRow(ctx, ` SELECT`+riExchangeDirectRowColumns+` FROM ri_exchange_direct_executions diff --git a/internal/config/store_postgres_ri_exchange_direct_list.go b/internal/config/store_postgres_ri_exchange_direct_list.go index 4f11d67c0..9cc6c8b55 100644 --- a/internal/config/store_postgres_ri_exchange_direct_list.go +++ b/internal/config/store_postgres_ri_exchange_direct_list.go @@ -39,21 +39,16 @@ func (s *PostgresStore) ListUnresolvedRIExchangeDirectExecutions(ctx context.Con if err != nil { return nil, err } + if err := requireDirectResolveSchema(ctx, s.db); err != nil { + return nil, err + } page := &RIExchangeDirectListPage{} if len(f.Allowed) == 0 { return page, nil } - accounts := make([]*string, len(f.Allowed)) - regions := make([]string, len(f.Allowed)) - for i, p := range f.Allowed { - accounts[i], regions[i] = p.CloudAccountID, p.Region - } + accounts, regions := allowedListArrays(f.Allowed) riID, region := optionalListText(normaliseRIExchangeSourceIDs([]string{f.RIID})), optionalListText([]string{f.Region}) - var afterAt *time.Time - var afterID *string - if f.After != nil { - afterAt, afterID = &f.After.CreatedAt, &f.After.ID - } + afterAt, afterID := listCursorArgs(f.After) rows, err := s.db.Query(ctx, ` SELECT`+riExchangeDirectRowColumns+` @@ -112,6 +107,25 @@ func validateRIExchangeDirectListFilter(f RIExchangeDirectListFilter) (limit int return limit, statuses, nil } +// allowedListArrays splits the allowed pairs into the two parallel arrays the +// query unnests. +func allowedListArrays(allowed []RIExchangeDirectAccountRegion) ([]*string, []string) { + accounts := make([]*string, len(allowed)) + regions := make([]string, len(allowed)) + for i, p := range allowed { + accounts[i], regions[i] = p.CloudAccountID, p.Region + } + return accounts, regions +} + +// listCursorArgs is the keyset cursor as nullable SQL arguments. +func listCursorArgs(after *RIExchangeDirectCursor) (*time.Time, *string) { + if after == nil { + return nil, nil + } + return &after.CreatedAt, &after.ID +} + // optionalListText is the first non-empty value as a nullable SQL argument. func optionalListText(vals []string) *string { if len(vals) == 0 || vals[0] == "" { diff --git a/internal/config/store_postgres_ri_exchange_direct_resolve.go b/internal/config/store_postgres_ri_exchange_direct_resolve.go index 07e90eff5..095944272 100644 --- a/internal/config/store_postgres_ri_exchange_direct_resolve.go +++ b/internal/config/store_postgres_ri_exchange_direct_resolve.go @@ -29,6 +29,9 @@ func (s *PostgresStore) ResolveRIExchangeDirectExecution(ctx context.Context, re if err := validateRIExchangeDirectResolve(req); err != nil { return nil, err } + if err := requireDirectResolveSchema(ctx, s.db); err != nil { + return nil, err + } res := &RIExchangeDirectResolveResult{ID: req.ID, ResolvedStatus: req.Outcome, Note: req.Note} err := s.WithTx(ctx, func(tx pgx.Tx) error { row, err := lockRIExchangeDirectForResolve(ctx, tx, req.ID) diff --git a/internal/config/store_postgres_ri_exchange_direct_schema_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_schema_integration_test.go new file mode 100644 index 000000000..468bb4597 --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_schema_integration_test.go @@ -0,0 +1,41 @@ +//go:build integration + +package config + +import ( + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The app deploys before migration 163 is applied (#1836): the guard and +// finalize must keep working without the resolution columns, and the operator +// paths must refuse rather than fail on a missing column. +func TestPostgresStoreDB_RIExchangeDirect_BeforeResolutionMigration(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + require.NoError(t, migrations.MigrateToVersion(env.ctx, env.poolA, getTestMigrationsPath(), migrationBelow(t, directResolveMigrationVersion))) + var cols int + require.NoError(t, env.poolA.QueryRow(env.ctx, `SELECT count(*) FROM information_schema.columns WHERE table_name = 'ri_exchange_direct_executions' AND column_name = 'resolved_at'`).Scan(&cols)) + require.Zero(t, cols, "precondition: the resolution columns are gone") + + held := env.directRow(t, "aws", "submitted", "ri-g1836-n1-held") + require.ErrorIs(t, env.reserve(t, testAccountID(836200000001), "ri-g1836-n1-held"), ErrRIExchangeSourceInFlight, "the guard still holds the RI") + require.NoError(t, env.reserve(t, testAccountID(836200000002), "ri-g1836-n1-free")) + + outcome := RIExchangeDirectOutcome{Status: RIExchangeDirectStatusCompleted, ProviderReference: "riex-1", + NetPayable: strPtr("40.25"), NetPayableCurrency: "USD"} + require.NoError(t, env.storeA.FinalizeRIExchangeDirectExecution(env.ctx, held, outcome)) + require.ErrorIs(t, env.storeA.FinalizeRIExchangeDirectExecution(env.ctx, held, outcome), ErrRIExchangeDirectExecutionNotSubmitted) + + _, err := env.storeA.ListUnresolvedRIExchangeDirectAccountRegions(env.ctx) + assert.ErrorIs(t, err, ErrRIExchangeDirectResolveNotReady) + _, err = env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, RIExchangeDirectListFilter{Limit: 10}) + assert.ErrorIs(t, err, ErrRIExchangeDirectResolveNotReady) + _, err = env.storeA.GetRIExchangeDirectExecution(env.ctx, held) + assert.ErrorIs(t, err, ErrRIExchangeDirectResolveNotReady) + _, err = env.resolve(t, directResolveReq(held, "failed", "submitted", time.Now().UTC())) + assert.ErrorIs(t, err, ErrRIExchangeDirectResolveNotReady) +} diff --git a/internal/config/store_postgres_ri_exchange_direct_schema_test.go b/internal/config/store_postgres_ri_exchange_direct_schema_test.go new file mode 100644 index 000000000..f77f7fba2 --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_schema_test.go @@ -0,0 +1,115 @@ +package config + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "testing" + "time" + + "github.com/pashagolub/pgxmock/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func expectDirectResolveVersion(mock pgxmock.PgxPoolIface, version int) { + mock.ExpectQuery("SELECT version, dirty FROM schema_migrations"). + WillReturnRows(pgxmock.NewRows([]string{"version", "dirty"}).AddRow(version, false)) +} + +// Before migration 163 the guard and finalize must not name resolved_at or +// resolved_status (#1836); the queries with it are pinned by the neighbouring tests. +func TestPGXMock_DirectResolveBeforeMigration_QueriesOmitResolutionColumns(t *testing.T) { + notReady := directResolveMigrationVersion - 1 + + t.Run("source guard", func(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + rec := validDirectExecution() + rec.Provider = "aws" + mock.ExpectBegin() + mock.ExpectExec("SELECT pg_advisory_xact_lock").WithArgs(riExchangeDailySpendLockKey). + WillReturnResult(pgxmock.NewResult("SELECT", 1)) + mock.ExpectQuery("SELECT id::text, ARRAY").WithArgs(anyArgsCfg(2)...). + WillReturnRows(pgxmock.NewRows([]string{"id", "shared"})) + expectDirectResolveVersion(mock, notReady) + mock.ExpectQuery(`status IN \('submitted', 'unknown'\)\s*\) d`).WithArgs([]string{"res-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()) + }) + + outcome := RIExchangeDirectOutcome{Status: RIExchangeDirectStatusCompleted, ProviderReference: "riex-1"} + t.Run("finalize", func(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + expectDirectResolveVersion(mock, notReady) + mock.ExpectExec(`UPDATE ri_exchange_direct_executions(.|\n)*AND status = 'submitted'$`).WithArgs(anyArgsCfg(7)...). + WillReturnResult(pgxmock.NewResult("UPDATE", 1)) + require.NoError(t, store.FinalizeRIExchangeDirectExecution(context.Background(), directExecTestID, outcome)) + assert.NoError(t, mock.ExpectationsWereMet()) + }) + + t.Run("finalize of a finalized row is refused", func(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + expectDirectResolveVersion(mock, notReady) + mock.ExpectExec("UPDATE ri_exchange_direct_executions").WithArgs(anyArgsCfg(7)...). + WillReturnResult(pgxmock.NewResult("UPDATE", 0)) + err := store.FinalizeRIExchangeDirectExecution(context.Background(), directExecTestID, outcome) + require.ErrorIs(t, err, ErrRIExchangeDirectExecutionNotSubmitted) + assert.NoError(t, mock.ExpectationsWereMet()) + }) + + t.Run("list, get and resolve refuse before touching the table", func(t *testing.T) { + for name, call := range map[string]func(*PostgresStore) error{ + "account regions": func(s *PostgresStore) error { + _, err := s.ListUnresolvedRIExchangeDirectAccountRegions(context.Background()) + return err + }, + "list": func(s *PostgresStore) error { + _, err := s.ListUnresolvedRIExchangeDirectExecutions(context.Background(), RIExchangeDirectListFilter{Limit: 10}) + return err + }, + "get": func(s *PostgresStore) error { + _, err := s.GetRIExchangeDirectExecution(context.Background(), directExecTestID) + return err + }, + "resolve": func(s *PostgresStore) error { + _, err := s.ResolveRIExchangeDirectExecution(context.Background(), RIExchangeDirectResolve{ID: directExecTestID, Outcome: "failed", ExpectedStatus: "submitted", Note: "n", CheckedAt: time.Now(), ResolvedBy: "ops"}) + return err + }, + } { + t.Run(name, func(t *testing.T) { + mock := newMock(t) + expectDirectResolveVersion(mock, notReady) + require.ErrorIs(t, call(storeWith(mock)), ErrRIExchangeDirectResolveNotReady) + assert.NoError(t, mock.ExpectationsWereMet()) + }) + } + }) +} + +// The constant must name this change's migration file, so a renumbering that +// forgets it fails here instead of failing closed in production. +func TestDirectResolveMigrationVersionNamesTheMigrationFile(t *testing.T) { + name := fmt.Sprintf("%06d_ri_exchange_direct_resolution.up.sql", directResolveMigrationVersion) + _, err := os.Stat(filepath.Join("..", "database", "postgres", "migrations", name)) + require.NoError(t, err, "directResolveMigrationVersion no longer matches the migration file") +} + +// A failed version read must not lose the provider outcome. +func TestPGXMock_FinalizeDirectExecution_UnreadableSchemaVersionStillFinalizes(t *testing.T) { + mock := newMock(t) + mock.ExpectQuery("SELECT version, dirty FROM schema_migrations").WillReturnError(errors.New("connection reset")) + mock.ExpectExec("UPDATE ri_exchange_direct_executions").WithArgs(anyArgsCfg(7)...). + WillReturnResult(pgxmock.NewResult("UPDATE", 1)) + outcome := RIExchangeDirectOutcome{Status: RIExchangeDirectStatusCompleted, ProviderReference: "riex-1"} + require.NoError(t, storeWith(mock).FinalizeRIExchangeDirectExecution(context.Background(), directExecTestID, outcome)) + assert.NoError(t, mock.ExpectationsWereMet()) +} diff --git a/internal/config/store_postgres_ri_exchange_direct_test.go b/internal/config/store_postgres_ri_exchange_direct_test.go index c87a6e9b5..94bfb6672 100644 --- a/internal/config/store_postgres_ri_exchange_direct_test.go +++ b/internal/config/store_postgres_ri_exchange_direct_test.go @@ -70,6 +70,7 @@ func TestPGXMock_SaveRIExchangeDirectExecution_AWSLocksAndGuardsBeforeInsert(t * 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"})) + expectDirectResolveVersion(mock, directResolveMigrationVersion) 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)...). @@ -135,6 +136,7 @@ func TestPGXMock_FinalizeRIExchangeDirectExecution_CompareAndSet(t *testing.T) { t.Run("submitted row is finalized", func(t *testing.T) { mock := newMock(t) store := storeWith(mock) + expectDirectResolveVersion(mock, directResolveMigrationVersion) mock.ExpectQuery(`UPDATE ri_exchange_direct_executions(.|\n)*WHERE id = \$1(.|\n)*AND status = 'submitted'(.|\n)*RETURNING resolved_status`). WithArgs(directExecTestID, "completed", pgxmock.AnyArg(), outcome.NetPayable, pgxmock.AnyArg(), pgxmock.AnyArg(), pgxmock.AnyArg()). WillReturnRows(pgxmock.NewRows([]string{"resolved_status"}).AddRow((*string)(nil))) @@ -145,6 +147,7 @@ func TestPGXMock_FinalizeRIExchangeDirectExecution_CompareAndSet(t *testing.T) { t.Run("finalized or missing row is refused", func(t *testing.T) { mock := newMock(t) store := storeWith(mock) + expectDirectResolveVersion(mock, directResolveMigrationVersion) mock.ExpectQuery("UPDATE ri_exchange_direct_executions"). WithArgs(anyArgsCfg(7)...). WillReturnRows(pgxmock.NewRows([]string{"resolved_status"})) diff --git a/internal/config/store_postgres_schema_version.go b/internal/config/store_postgres_schema_version.go index 731feba5d..2203bedec 100644 --- a/internal/config/store_postgres_schema_version.go +++ b/internal/config/store_postgres_schema_version.go @@ -12,6 +12,11 @@ import ( // columns to purchase_executions (issue #999). const settlementMigrationVersion = 155 +// directResolveMigrationVersion is the migration that adds the resolution +// columns to ri_exchange_direct_executions (#1836). It mirrors +// directResolutionMigration in internal/database/postgres/migrations. +const directResolveMigrationVersion = 163 + // rowQuerier is satisfied by both the store's connection and a pgx.Tx, so a // schema check can run inside the caller's transaction. type rowQuerier interface { @@ -45,3 +50,28 @@ func settlementSchemaReady(ctx context.Context, q rowQuerier) (bool, error) { } return version > settlementMigrationVersion || (version == settlementMigrationVersion && !dirty), nil } + +// directResolveSchemaReady reports whether the direct-exchange resolution +// columns exist. Same N-1 rule as settlementSchemaReady: until migration 163 is +// applied the guard and finalize drop their resolved_at references and the +// list, get and resolve paths answer ErrRIExchangeDirectResolveNotReady. +func directResolveSchemaReady(ctx context.Context, q rowQuerier) (bool, error) { + version, dirty, found, err := readSchemaVersion(ctx, q) + if err != nil || !found { + return false, err + } + return version > directResolveMigrationVersion || (version == directResolveMigrationVersion && !dirty), nil +} + +// requireDirectResolveSchema fails with ErrRIExchangeDirectResolveNotReady +// while the resolution columns are missing. +func requireDirectResolveSchema(ctx context.Context, q rowQuerier) error { + ready, err := directResolveSchemaReady(ctx, q) + if err != nil { + return err + } + if !ready { + return ErrRIExchangeDirectResolveNotReady + } + return nil +} diff --git a/internal/config/store_postgres_schema_version_test.go b/internal/config/store_postgres_schema_version_test.go index dc51d7474..045c5123a 100644 --- a/internal/config/store_postgres_schema_version_test.go +++ b/internal/config/store_postgres_schema_version_test.go @@ -53,3 +53,28 @@ func TestSettlementSchemaReady(t *testing.T) { }) } } + +// The direct-resolve paths run before migration 163 is applied (#1836). +func TestDirectResolveSchemaReady(t *testing.T) { + t.Parallel() + v := directResolveMigrationVersion + cases := []struct { + name string + row fakeVersionRow + want bool + }{ + {"nothing applied", fakeVersionRow{err: pgx.ErrNoRows}, false}, + {"before the migration", fakeVersionRow{version: v - 1}, false}, + {"at the migration", fakeVersionRow{version: v}, true}, + {"at the migration, dirty", fakeVersionRow{version: v, dirty: true}, false}, + {"after it, even with a later dirty migration", fakeVersionRow{version: v + 1, dirty: true}, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := directResolveSchemaReady(context.Background(), fakeVersionQuerier{tc.row}) + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} From f96bd5ebcd827446ac4be3554f059eb3f7a830d0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 16:33:03 +0100 Subject: [PATCH 26/30] fix(api): answer 503 from the direct-exchange list before migration 163 The list reads the account-region pairs first, and its not-ready error came back unmapped as a generic 500 while resolve already answered 503. Map it in the list handler too and pin both store reads. Refs #1836 Co-Authored-By: Claude Sonnet 5.5 --- .../api/handler_ri_exchange_direct_list.go | 4 +- ...ri_exchange_direct_list_failclosed_test.go | 51 +++++++++++++------ 2 files changed, 38 insertions(+), 17 deletions(-) diff --git a/internal/api/handler_ri_exchange_direct_list.go b/internal/api/handler_ri_exchange_direct_list.go index 876503d8b..8c4f093ee 100644 --- a/internal/api/handler_ri_exchange_direct_list.go +++ b/internal/api/handler_ri_exchange_direct_list.go @@ -25,8 +25,8 @@ func (h *Handler) listRIExchangeDirectExecutions(ctx context.Context, req *event return nil, NewClientError(503, "RI exchange resolve storage is unavailable") } allowed, err := h.visibleRIExchangeDirectPairs(ctx, session, store) - if err != nil { - return nil, err + if mapped := mapRIExchangeResolveError(err); mapped != nil { + return nil, mapped } filter.Allowed = allowed page, err := store.ListUnresolvedRIExchangeDirectExecutions(ctx, filter) diff --git a/internal/api/handler_ri_exchange_direct_list_failclosed_test.go b/internal/api/handler_ri_exchange_direct_list_failclosed_test.go index b75aeba3e..b6586961c 100644 --- a/internal/api/handler_ri_exchange_direct_list_failclosed_test.go +++ b/internal/api/handler_ri_exchange_direct_list_failclosed_test.go @@ -3,6 +3,7 @@ package api import ( "context" "errors" + "fmt" "testing" "github.com/aws/aws-lambda-go/events" @@ -32,6 +33,22 @@ func (s *failingDirectListStore) ListUnresolvedRIExchangeDirectExecutions(ctx co return s.directListStore.ListUnresolvedRIExchangeDirectExecutions(ctx, f) } +func callDirectList(t *testing.T, store *failingDirectListStore, authErr error) (any, error) { + t.Helper() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "tok").Return(&Session{UserID: apgUserID}, nil).Maybe() + mockAuth.On("HasPermissionAPI", mock.Anything, mock.Anything, "execute", "ri-exchange").Return(true, nil).Maybe() + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return([]string(nil), nil).Maybe() + if authErr != nil { + mockAuth.On("HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, "execute", "ri-exchange", mock.Anything).Return(false, authErr) + } else { + mockAuth.On("HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, "execute", "ri-exchange", mock.Anything).Return(true, nil).Maybe() + } + h := &Handler{config: store, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer tok"}} + return h.listRIExchangeDirectExecutions(context.Background(), req) +} + // A failure anywhere in the visibility chain is a server error with no // partial list: nothing is shown on a guess. func TestListRIExchangeDirectExecutions_FailsClosed(t *testing.T) { @@ -40,21 +57,7 @@ func TestListRIExchangeDirectExecutions_FailsClosed(t *testing.T) { dlRow(dlID2, dlPtr(dlAcctB), "eu-west-1", "unknown", 2), } boom := errors.New("backend down") - call := func(t *testing.T, store *failingDirectListStore, authErr error) (any, error) { - t.Helper() - mockAuth := new(MockAuthService) - mockAuth.On("ValidateSession", mock.Anything, "tok").Return(&Session{UserID: apgUserID}, nil).Maybe() - mockAuth.On("HasPermissionAPI", mock.Anything, mock.Anything, "execute", "ri-exchange").Return(true, nil).Maybe() - mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return([]string(nil), nil).Maybe() - if authErr != nil { - mockAuth.On("HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, "execute", "ri-exchange", mock.Anything).Return(false, authErr) - } else { - mockAuth.On("HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, "execute", "ri-exchange", mock.Anything).Return(true, nil).Maybe() - } - h := &Handler{config: store, auth: mockAuth} - req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer tok"}} - return h.listRIExchangeDirectExecutions(context.Background(), req) - } + call := callDirectList tests := []struct { name string store *failingDirectListStore @@ -79,3 +82,21 @@ func TestListRIExchangeDirectExecutions_FailsClosed(t *testing.T) { assert.Equal(t, []string{dlID1, dlID2}, dlIDs(t, out)) }) } + +// Before migration 163 the list answers 503, whichever of its two store reads +// notices first; it is not a generic 500. +func TestListRIExchangeDirectExecutions_NotReadyIs503(t *testing.T) { + rows := []config.RIExchangeDirectRow{dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1)} + notReady := fmt.Errorf("list unresolved direct exchange rows: %w", config.ErrRIExchangeDirectResolveNotReady) + for name, store := range map[string]*failingDirectListStore{ + "the pair query": {directListStore: dlStore(rows...), pairsErr: notReady}, + "the page query": {directListStore: dlStore(rows...), listErr: notReady}, + } { + t.Run(name, func(t *testing.T) { + out, err := callDirectList(t, store, nil) + require.Error(t, err) + assert.Equal(t, 503, clientCode(t, err)) + assert.Nil(t, out) + }) + } +} From dc2aabdb8724642be5c7ee53c824a7376cef1dc0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 16:33:19 +0100 Subject: [PATCH 27/30] test(config): pin the guard's schema-version read failure and fix the lint findings An unreadable schema version must refuse the reservation and must look neither like a held source RI nor like a pre-migration schema; the test tells the two failure sites apart by their log lines. Also name the helper results, stop shadowing err in the list store method and fix a misspelling. Refs #1836 Co-Authored-By: Claude Sonnet 5.5 --- .../store_postgres_ri_exchange_direct_list.go | 12 +++---- ...postgres_ri_exchange_direct_schema_test.go | 32 ++++++++++++++++++- 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/internal/config/store_postgres_ri_exchange_direct_list.go b/internal/config/store_postgres_ri_exchange_direct_list.go index 9cc6c8b55..d3e7278b9 100644 --- a/internal/config/store_postgres_ri_exchange_direct_list.go +++ b/internal/config/store_postgres_ri_exchange_direct_list.go @@ -39,8 +39,8 @@ func (s *PostgresStore) ListUnresolvedRIExchangeDirectExecutions(ctx context.Con if err != nil { return nil, err } - if err := requireDirectResolveSchema(ctx, s.db); err != nil { - return nil, err + if schemaErr := requireDirectResolveSchema(ctx, s.db); schemaErr != nil { + return nil, schemaErr } page := &RIExchangeDirectListPage{} if len(f.Allowed) == 0 { @@ -109,9 +109,9 @@ func validateRIExchangeDirectListFilter(f RIExchangeDirectListFilter) (limit int // allowedListArrays splits the allowed pairs into the two parallel arrays the // query unnests. -func allowedListArrays(allowed []RIExchangeDirectAccountRegion) ([]*string, []string) { - accounts := make([]*string, len(allowed)) - regions := make([]string, len(allowed)) +func allowedListArrays(allowed []RIExchangeDirectAccountRegion) (accounts []*string, regions []string) { + accounts = make([]*string, len(allowed)) + regions = make([]string, len(allowed)) for i, p := range allowed { accounts[i], regions[i] = p.CloudAccountID, p.Region } @@ -119,7 +119,7 @@ func allowedListArrays(allowed []RIExchangeDirectAccountRegion) ([]*string, []st } // listCursorArgs is the keyset cursor as nullable SQL arguments. -func listCursorArgs(after *RIExchangeDirectCursor) (*time.Time, *string) { +func listCursorArgs(after *RIExchangeDirectCursor) (at *time.Time, id *string) { if after == nil { return nil, nil } diff --git a/internal/config/store_postgres_ri_exchange_direct_schema_test.go b/internal/config/store_postgres_ri_exchange_direct_schema_test.go index f77f7fba2..c6b55cb72 100644 --- a/internal/config/store_postgres_ri_exchange_direct_schema_test.go +++ b/internal/config/store_postgres_ri_exchange_direct_schema_test.go @@ -1,6 +1,7 @@ package config import ( + "bytes" "context" "errors" "fmt" @@ -9,6 +10,7 @@ import ( "testing" "time" + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/pashagolub/pgxmock/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -20,7 +22,7 @@ func expectDirectResolveVersion(mock pgxmock.PgxPoolIface, version int) { } // Before migration 163 the guard and finalize must not name resolved_at or -// resolved_status (#1836); the queries with it are pinned by the neighbouring tests. +// resolved_status (#1836); the queries with it are pinned by the neighboring tests. func TestPGXMock_DirectResolveBeforeMigration_QueriesOmitResolutionColumns(t *testing.T) { notReady := directResolveMigrationVersion - 1 @@ -113,3 +115,31 @@ func TestPGXMock_FinalizeDirectExecution_UnreadableSchemaVersionStillFinalizes(t require.NoError(t, storeWith(mock).FinalizeRIExchangeDirectExecution(context.Background(), directExecTestID, outcome)) assert.NoError(t, mock.ExpectationsWereMet()) } + +// A failed version read in the guard refuses the reservation: reading it as +// "not ready" would drop the resolved_at filter and reading it as "ready" would +// name a column that may not exist. Neither may look like a held source RI. +func TestPGXMock_DirectGuard_UnreadableSchemaVersionRefuses(t *testing.T) { + var logs bytes.Buffer + prev := logging.SetOutput(&logs) + t.Cleanup(func() { logging.SetOutput(prev) }) + + mock := newMock(t) + rec := validDirectExecution() + rec.Provider = "aws" + mock.ExpectBegin() + mock.ExpectExec("SELECT pg_advisory_xact_lock").WithArgs(riExchangeDailySpendLockKey). + WillReturnResult(pgxmock.NewResult("SELECT", 1)) + mock.ExpectQuery("SELECT id::text, ARRAY").WithArgs(anyArgsCfg(2)...). + WillReturnRows(pgxmock.NewRows([]string{"id", "shared"})) + mock.ExpectQuery("SELECT version, dirty FROM schema_migrations").WillReturnError(errors.New("connection reset")) + mock.ExpectRollback() + + err := storeWith(mock).SaveRIExchangeDirectExecution(context.Background(), rec) + + require.Error(t, err) + assert.NotErrorIs(t, err, ErrRIExchangeSourceInFlight) + assert.Contains(t, logs.String(), "schema version check failed", "the refusal comes from the version read, not from the blocker query") + assert.NotContains(t, logs.String(), "source RI check failed") + assert.NoError(t, mock.ExpectationsWereMet()) +} From dd4e4c3d0dccfb12ef12f820ffe407166b44ac74 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 16:33:32 +0100 Subject: [PATCH 28/30] docs: record that direct finalize reads the schema version outside its statement Refs #1836 Co-Authored-By: Claude Sonnet 5.5 --- known-issues.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/known-issues.md b/known-issues.md index b0fe11787..64be58531 100644 --- a/known-issues.md +++ b/known-issues.md @@ -145,6 +145,15 @@ 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. +### Direct finalize reads the schema version outside its statement (issue #1836) + +`FinalizeRIExchangeDirectExecution` reads the schema version before its UPDATE, +not in the same statement. If migration 163 lands between the two, a late +provider outcome that contradicts an operator resolution is finalized without +the contradiction being logged (the row keeps both facts either way). A version +read that fails is logged and finalizes without the check. Neither loses the +provider outcome. + ### RI id matching is exact for stored history rows (issue #1829) The direct-execute guard normalises (lower, trim) its input and the stored direct From cb3fa5b6b1e5abc11daf5993c57605534955bce0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 16:45:36 +0100 Subject: [PATCH 29/30] fix(api): admit a direct row by its AWS account number and key replay on the status seen The scope check compared only the row's internal cloud account id, so a group that lists an account by its AWS account number (the group write keeps an external id verbatim) did not see that account's stuck rows in the list and got a 404 on resolve. Either form now admits the row; external ids are unique per provider, so the number cannot name another AWS account. The resolution note now records expected=, so a repeat that differs only in expected_status is a conflict, while an identical retry stays a replay even after a late finalize moved the row's status. Co-Authored-By: Claude Sonnet 5.5 --- .../api/handler_ri_exchange_direct_list.go | 39 +++++++++++--- .../handler_ri_exchange_direct_list_test.go | 32 +++++++++++ .../api/handler_ri_exchange_direct_resolve.go | 6 ++- ...dler_ri_exchange_direct_resolve_request.go | 7 ++- ...handler_ri_exchange_direct_resolve_test.go | 53 ++++++++++++++++++- internal/api/openapi.yaml | 3 +- .../ri-exchange-direct-execution-settle.md | 3 +- 7 files changed, 130 insertions(+), 13 deletions(-) diff --git a/internal/api/handler_ri_exchange_direct_list.go b/internal/api/handler_ri_exchange_direct_list.go index 8c4f093ee..28eba6f42 100644 --- a/internal/api/handler_ri_exchange_direct_list.go +++ b/internal/api/handler_ri_exchange_direct_list.go @@ -24,7 +24,11 @@ func (h *Handler) listRIExchangeDirectExecutions(ctx context.Context, req *event if !ok { return nil, NewClientError(503, "RI exchange resolve storage is unavailable") } - allowed, err := h.visibleRIExchangeDirectPairs(ctx, session, store) + accounts, err := h.riExchangeDirectAccounts(ctx) + if err != nil { + return nil, err + } + allowed, err := h.visibleRIExchangeDirectPairs(ctx, session, store, accounts) if mapped := mapRIExchangeResolveError(err); mapped != nil { return nil, mapped } @@ -51,7 +55,7 @@ func (h *Handler) listRIExchangeDirectExecutions(ctx context.Context, req *event // the direct execute checks. The grant is evaluated by the authorization // service, never reimplemented here, so OR'd grants, a user API key's own // narrower permissions and region spelling behave as they do on execute. -func (h *Handler) visibleRIExchangeDirectPairs(ctx context.Context, session *Session, store config.RIExchangeDirectListStore) ([]config.RIExchangeDirectAccountRegion, error) { +func (h *Handler) visibleRIExchangeDirectPairs(ctx context.Context, session *Session, store config.RIExchangeDirectListStore, accounts map[string]config.CloudAccount) ([]config.RIExchangeDirectAccountRegion, error) { scope, err := h.getAccountScope(ctx, session) if err != nil { return nil, fmt.Errorf("failed to get allowed accounts: %w", err) @@ -62,7 +66,7 @@ func (h *Handler) visibleRIExchangeDirectPairs(ctx context.Context, session *Ses } var visible []config.RIExchangeDirectAccountRegion for _, p := range pairs { - account, inScope := riExchangeDirectPairAccount(scope, p) + account, inScope := riExchangeDirectPairAccount(scope, accounts, p) if !inScope { continue } @@ -77,15 +81,38 @@ func (h *Handler) visibleRIExchangeDirectPairs(ctx context.Context, session *Ses return visible, nil } +// riExchangeDirectAccounts loads the registered AWS accounts by ID, so the scope +// check can also match an account's AWS account number. +func (h *Handler) riExchangeDirectAccounts(ctx context.Context) (map[string]config.CloudAccount, error) { + provider := string(common.ProviderAWS) + accounts, err := h.config.ListCloudAccounts(ctx, config.CloudAccountFilter{Provider: &provider}) + if err != nil { + return nil, fmt.Errorf("failed to list cloud accounts: %w", err) + } + byID := make(map[string]config.CloudAccount, len(accounts)) + for i := range accounts { + byID[accounts[i].ID] = accounts[i] + } + return byID, nil +} + // riExchangeDirectPairAccount returns the account the grant is checked // against (the unattributed sentinel for a row with no registered account) and // whether the session's account scope covers it. A row with no account needs -// an unrestricted scope. -func riExchangeDirectPairAccount(scope auth.AccountScope, p config.RIExchangeDirectAccountRegion) (string, bool) { +// an unrestricted scope. A group may list an account by its ID or by its AWS +// account number (the group write keeps an external id verbatim), so either +// form admits the row; external ids are unique per provider, so the number +// cannot name another AWS account. +func riExchangeDirectPairAccount(scope auth.AccountScope, accounts map[string]config.CloudAccount, p config.RIExchangeDirectAccountRegion) (string, bool) { if p.CloudAccountID == nil { return unattributedAccountConstraint, scope.AllowsAll() } - return *p.CloudAccountID, scope.Allows(*p.CloudAccountID, "") + id := *p.CloudAccountID + if scope.Allows(id, "") { + return id, true + } + external := accounts[id].ExternalID + return id, external != "" && scope.Allows(external, "") } // riExchangeDirectPairGranted asks the authorization service whether the diff --git a/internal/api/handler_ri_exchange_direct_list_test.go b/internal/api/handler_ri_exchange_direct_list_test.go index 94759d305..5c7e62b47 100644 --- a/internal/api/handler_ri_exchange_direct_list_test.go +++ b/internal/api/handler_ri_exchange_direct_list_test.go @@ -248,3 +248,35 @@ func TestListRIExchangeDirectExecutions_NoStore503(t *testing.T) { &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer tok"}}) assert.Equal(t, 503, clientCode(t, err)) } + +// A group may list an account by its AWS account number. The list must admit +// the row of the account that number names, not hide it because only the +// internal id was compared. +func TestListRIExchangeDirectExecutions_AllowedAccountsMayNameTheAWSAccountNumber(t *testing.T) { + rows := []config.RIExchangeDirectRow{ + dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1), + dlRow(dlID2, dlPtr(dlAcctB), "eu-west-1", "unknown", 2), + } + accounts := []config.CloudAccount{ + {ID: dlAcctA, Provider: "aws", ExternalID: "111122223333"}, + {ID: dlAcctB, Provider: "aws", ExternalID: "999999999999"}, + } + for name, tc := range map[string]struct { + allowed []string + want []string + }{ + "the account number": {[]string{"111122223333"}, []string{dlID1}}, + "the internal id": {[]string{dlAcctA}, []string{dlID1}}, + "the other account's number": {[]string{"999999999999"}, []string{dlID2}}, + "a number that is nobody's": {[]string{"000000000000"}, nil}, + "both, one by number and one by id": {[]string{"111122223333", dlAcctB}, []string{dlID1, dlID2}}, + } { + t.Run(name, func(t *testing.T) { + store := dlStore(rows...) + store.ListCloudAccountsFn = func(context.Context, config.CloudAccountFilter) ([]config.CloudAccount, error) { return accounts, nil } + out, err := dlCall(t, store, riExecute, tc.allowed, nil) + require.NoError(t, err) + assert.ElementsMatch(t, tc.want, dlIDs(t, out)) + }) + } +} diff --git a/internal/api/handler_ri_exchange_direct_resolve.go b/internal/api/handler_ri_exchange_direct_resolve.go index 054fb1d7c..67311df6a 100644 --- a/internal/api/handler_ri_exchange_direct_resolve.go +++ b/internal/api/handler_ri_exchange_direct_resolve.go @@ -216,7 +216,11 @@ func (h *Handler) authorizeRIExchangeDirectRow(ctx context.Context, session *Ses if err != nil { return fmt.Errorf("failed to get allowed accounts: %w", err) } - account, inScope := riExchangeDirectPairAccount(scope, config.RIExchangeDirectAccountRegion{CloudAccountID: row.CloudAccountID, Region: row.Region}) + accounts, err := h.riExchangeDirectAccounts(ctx) + if err != nil { + return err + } + account, inScope := riExchangeDirectPairAccount(scope, accounts, config.RIExchangeDirectAccountRegion{CloudAccountID: row.CloudAccountID, Region: row.Region}) if !inScope { return errNotFound } diff --git a/internal/api/handler_ri_exchange_direct_resolve_request.go b/internal/api/handler_ri_exchange_direct_resolve_request.go index 90e68429a..b904f6163 100644 --- a/internal/api/handler_ri_exchange_direct_resolve_request.go +++ b/internal/api/handler_ri_exchange_direct_resolve_request.go @@ -92,9 +92,12 @@ func (b riExchangeDirectResolveRequest) checkedAt(rowRegion string) (time.Time, // note is the exact text stored in resolution_note. The fields are in a fixed // order and quoted, so a repeat of the same request produces the same string -// and a different one cannot. +// and a different one cannot. That includes the status the operator saw: a +// repeat that differs only in expected_status is a conflict, while an identical +// retry stays a replay even after a late finalize moved the row's status. func (b riExchangeDirectResolveRequest) note(checkedAt time.Time) string { - s := fmt.Sprintf("operator:%s: region=%q checked_at=%s", b.Outcome, b.CloudCheck.Region, checkedAt.Format(time.RFC3339Nano)) + s := fmt.Sprintf("operator:%s: region=%q checked_at=%s expected=%s", + b.Outcome, b.CloudCheck.Region, checkedAt.Format(time.RFC3339Nano), b.ExpectedStatus) if b.Outcome == config.RIExchangeDirectStatusFailed { s += " ack=true" } diff --git a/internal/api/handler_ri_exchange_direct_resolve_test.go b/internal/api/handler_ri_exchange_direct_resolve_test.go index d2aac9b3e..ebf3fb326 100644 --- a/internal/api/handler_ri_exchange_direct_resolve_test.go +++ b/internal/api/handler_ri_exchange_direct_resolve_test.go @@ -117,7 +117,7 @@ func TestResolveRIExchangeDirect_PassesOperatorDecisionToStore(t *testing.T) { require.NotNil(t, got.ResolvedByUserID) assert.Equal(t, apgUserID, *got.ResolvedByUserID) assert.Equal(t, "ops@example.com", got.ResolvedBy) - assert.Equal(t, `operator:failed: region="eu-west-1" checked_at=2026-10-19T08:00:00Z ack=true reason="source RIs still active in the EC2 console"`, got.Note) + assert.Equal(t, `operator:failed: region="eu-west-1" checked_at=2026-10-19T08:00:00Z expected=unknown ack=true reason="source RIs still active in the EC2 console"`, got.Note) resp, ok := out.(riExchangeDirectResolveResponse) require.True(t, ok) assert.Equal(t, riExchangeDirectResolveResponse{ID: dlID1, Status: "unknown", ResolvedStatus: "failed", @@ -129,7 +129,7 @@ func TestResolveRIExchangeDirect_PassesOperatorDecisionToStore(t *testing.T) { completed := drStore() _, err = drCaller{perms: riExecute}.call(completed, dlID1, drBody("completed", "")) require.NoError(t, err) - assert.Equal(t, `operator:completed: region="eu-west-1" checked_at=2026-10-19T08:00:00Z reason="source RIs still active in the EC2 console"`, completed.got.Note) + assert.Equal(t, `operator:completed: region="eu-west-1" checked_at=2026-10-19T08:00:00Z expected=unknown reason="source RIs still active in the EC2 console"`, completed.got.Note) } // The store replays only a byte-identical note and refuses any other as a @@ -203,3 +203,52 @@ func TestResolveRIExchangeDirect_Needs503WithoutTheStore(t *testing.T) { _, err := h.resolveRIExchangeDirectExecution(context.Background(), req, dlID1) assert.Equal(t, 503, clientCode(t, err)) } + +// The status the operator saw is part of the replay identity: a repeat that +// changes only expected_status must not read as the same request. +func TestResolveRIExchangeDirect_NoteDiffersByExpectedStatus(t *testing.T) { + body := func(expected string) string { + return fmt.Sprintf(`{"outcome":"failed","expected_status":%q,"reason":"source RIs still active in the EC2 console",`+ + `"acknowledge_unknown_outcome":true,"cloud_check":{"checked_region":%q,"checked_at":%q}}`, + expected, drRegion, riResolveAt.Format(time.RFC3339)) + } + unknown, submitted := drStore(), drStore() + _, err := drCaller{perms: riExecute}.call(unknown, dlID1, body("unknown")) + require.NoError(t, err) + _, err = drCaller{perms: riExecute}.call(submitted, dlID1, body("submitted")) + require.NoError(t, err) + assert.Contains(t, unknown.got.Note, "expected=unknown") + assert.Contains(t, submitted.got.Note, "expected=submitted") + assert.NotEqual(t, unknown.got.Note, submitted.got.Note) +} + +// A group may list an account by its AWS account number, which the group write +// keeps verbatim. The row's registered account must be admitted by that entry, +// as by its internal id; another account's number must not admit it. +func TestResolveRIExchangeDirect_AllowedAccountsMayNameTheAWSAccountNumber(t *testing.T) { + accounts := []config.CloudAccount{ + {ID: dlAcctA, Provider: "aws", ExternalID: "111122223333"}, + {ID: dlAcctB, Provider: "aws", ExternalID: "999999999999"}, + } + for name, tc := range map[string]struct { + allowed []string + denied bool + }{ + "by the account number": {[]string{"111122223333"}, false}, + "by the internal id": {[]string{dlAcctA}, false}, + "another account's number": {[]string{"999999999999"}, true}, + "a number that is nobody's": {[]string{"000000000000"}, true}, + } { + t.Run(name, func(t *testing.T) { + store := drStore() + store.ListCloudAccountsFn = func(context.Context, config.CloudAccountFilter) ([]config.CloudAccount, error) { return accounts, nil } + _, err := drCaller{perms: riExecute, accounts: tc.allowed}.call(store, dlID1, drFailedBody) + if !tc.denied { + require.NoError(t, err) + return + } + require.ErrorIs(t, err, errNotFound, "a row outside the caller's accounts reads as not found") + assert.Zero(t, store.called) + }) + } +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index d473505ba..18943e73a 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -3029,7 +3029,8 @@ paths: be at or after that time (for an `unknown` row, one minute after it completed) and not in the future. `expected_status` must be the status the row has now. A repeat of the same request returns the stored - result with `replayed: true`. A row outside the session's allowed + result with `replayed: true`; a repeat that differs in any field, + including `expected_status`, is a conflict. A row outside the session's allowed accounts reads as not found. parameters: - $ref: '#/components/parameters/CSRFToken' diff --git a/runbooks/ri-exchange-direct-execution-settle.md b/runbooks/ri-exchange-direct-execution-settle.md index c37a04694..d6f088234 100644 --- a/runbooks/ri-exchange-direct-execution-settle.md +++ b/runbooks/ri-exchange-direct-execution-settle.md @@ -65,7 +65,8 @@ curl -s -X POST \ Non-ASCII text needs fewer characters; the API answers 400 "reason too long once encoded; shorten it". - `checked_region` must be the row's region. A row with an empty region cannot be resolved through the API (400 on region); use the break-glass statement below. -- A repeat of the same request returns the stored result with `replayed: true`. +- A repeat of the same request returns the stored result with `replayed: true`. A repeat that + differs in any field, including `expected_status`, is a 409, not a replay. - 409 with "wait until" and a time means the row is too young; see "Why the age" below. - 400 "checked_at must be at or after" and a time means your AWS check is too early; see "Why checked_at has a lower bound" below. Check again at AWS, then retry with the new time. From e75c742e4b5760131634c0c253c0626cfb782088 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 17:33:56 +0100 Subject: [PATCH 30/30] test(config): pin the guard's own error when the schema version cannot be read The test passed even if the guard swallowed the version-read error, because the Save that continued then failed on an unexpected mock insert. Assert the guard's own refusal text so that mutation fails. Refs #1836 Co-Authored-By: Claude Sonnet 5.5 --- .../config/store_postgres_ri_exchange_direct_schema_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/config/store_postgres_ri_exchange_direct_schema_test.go b/internal/config/store_postgres_ri_exchange_direct_schema_test.go index c6b55cb72..76ce4e226 100644 --- a/internal/config/store_postgres_ri_exchange_direct_schema_test.go +++ b/internal/config/store_postgres_ri_exchange_direct_schema_test.go @@ -137,7 +137,7 @@ func TestPGXMock_DirectGuard_UnreadableSchemaVersionRefuses(t *testing.T) { err := storeWith(mock).SaveRIExchangeDirectExecution(context.Background(), rec) - require.Error(t, err) + require.ErrorContains(t, err, "failed to check RI exchange source RIs held by a direct execute") assert.NotErrorIs(t, err, ErrRIExchangeSourceInFlight) assert.Contains(t, logs.String(), "schema version check failed", "the refusal comes from the version read, not from the blocker query") assert.NotContains(t, logs.String(), "source RI check failed")