[IAmJSD]

Root cause

Exchanges executed directly through the API left no record. Neither executeExchange (AWS, POST /api/ri-exchange/execute) nor executeAzureExchange (POST /api/ri-exchange/azure-instances/exchange) wrote anything; only the scheduled approve/reject flow persisted, to ri_exchange_history. On current main both handlers went straight from the #1642 submit claim to the provider call, and from there back to the response.

Closes #82

Decision: separate table, not ri_exchange_history

The issue asks us to decide this first. A new table, ri_exchange_direct_executions (migration 000131), is used because ri_exchange_history is not a passive log:

• its account_id has CHECK (account_id ~ '^\d{12}$'), so an Azure exchange (subscription GUID) can't be stored there at all;
• its processing/completed rows are the approval flow's daily-spend ledger (riExchangeDailySpendQuery), and its processing rows are swept by GetStaleProcessingExchanges. Putting direct executes there would quietly change the approval flow's cap and reaper behavior. That might be wanted later, but it's a policy decision, not an audit fix.

What gets recorded

Per the issue's list: provider, cloud_account_id, Azure subscription (provider_account_id), AWS region, source_ids (plus sources JSON with Azure's per-source quantity, since a partial exchange of a reservation matters), targets JSON (AWS offering + count, with the legacy singleton recorded as a one-element list; Azure SKU/location/term/quantity), the caller's cap and currency, Azure's pre-execute re-quote (quoted_net_payable), the AWS exchange ID or Azure session ID (provider_reference), net payable + currency as reported, Azure's operation status, the error text, the actor (executed_by_user_id, executed_by = email or the admin-API-key actor, executed_by_api_key_id), and created_at/completed_at from the DB clock. There are no FKs, so the trail outlives deleted users, keys and accounts.

Flow and statuses

1. After every gate and guardrail, beginDirectExchange inserts the row as submitted, then takes the #1642 claim. If the insert fails, the exchange is refused and no claim is taken, so an honest retry isn't blocked for 15 minutes. If the claim is refused (a duplicate inside the window, or a claim-store error), the row is finalized failed ("not submitted to the provider").
2. After the provider call, finalizeDirectExchange records the outcome once, on ctxutil.DetachedWithTimeout(ctx, riExchangeTerminalSaveTimeout), the same detached, bounded pattern as the approval flow's terminal write (fix(ri-exchange): run ExecuteExchange outside the request context #790). The Lambda budget is unchanged: 4m execution + 30s terminal write. A finalize failure is logged and does not turn a committed exchange into an error response; the row stays submitted, which is the "outcome never recorded" signal.
3. Statuses:
   • completed: the provider reported success.
   • failed: the handler mapped the error to a 4xx (provider client fault), or the request was refused before reaching the provider.
   • unknown: any other error (timeout, 5xx, transport), which doesn't prove the exchange was rejected. This is deliberately pessimistic. AWS pkg/exchange.ExecuteExchange returns untyped errors for its own pre-Accept guardrail refusals (e.g. "paymentDue … exceeds max"), and the handler can't tell those from a post-Accept failure without string matching, so they're recorded as unknown, with the quoted amount and the raw error text.
4. FinalizeRIExchangeDirectExecution is a compare-and-set (WHERE id = $1 AND status = 'submitted'). A second finalize, whether concurrent or later, updates nothing and returns ErrRIExchangeDirectExecutionNotSubmitted. Table CHECKs enforce (status = 'submitted') = (completed_at IS NULL), the four statuses, the providers, non-empty source and target arrays, and a non-empty actor.

Amounts are canonicalized from the parsed big.Rat before the NUMERIC cast. ParseDecimalRat accepts things like 1/3 and 0x10 that Postgres would reject, and without this an odd-but-valid cap would make the audit insert fail and refuse the exchange.

Concurrency

• Two concurrent identical submits each insert their own row (fresh UUIDs, no contention). The claim lets exactly one through, and the other row is finalized failed.
• Finalize races are tested on real Postgres. One transaction holds an uncommitted finalize while store.FinalizeRIExchangeDirectExecution runs concurrently. The test asserts it blocks on the row lock (it doesn't answer within 300ms), then, after commit, returns ErrRIExchangeDirectExecutionNotSubmitted, and the row keeps the first outcome.
• Mutation probe: dropping AND status = 'submitted' from the UPDATE fails a_finalized_row_is_never_rewritten, a_finalize_racing_an_uncommitted_finalize_keeps_the_first_outcome and the pgxmock CAS test.

Verification

All evidence is local unit, mock, pgxmock or real-Postgres (testcontainers) evidence. No real AWS or Azure calls were made: provider calls are injected (executeExchangeFn, mockAzureExchangeOpsClient). Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid.

Fail-before / pass-after (handler level). I reverted only internal/api/handler_ri_exchange.go to origin/main, keeping the new tests and helpers, and ran:

go test ./internal/api/ -count=1 -run 'TestExecuteExchange_DirectExecuteWritesAuditTrail|TestExecuteAzureExchange_DirectExecuteWritesAuditTrail|TestExecuteExchange_AuditRowWrittenBeforeClaimAndCommit|TestExecuteExchange_AuditInsertFailureRefusesExchange|TestExecuteAzureExchange_AuditInsertFailureRefusesExchange|FailureOutcomeClassification|DuplicateSubmitRecordedAsRefused|OutcomeRecordedAfterRequestCancel'

• before: all 9 FAIL ("[]" should have 1 item(s), but has 0: no audit row written)
• after: all PASS

Commands run on the pushed SHA:

go build ./... && go vet ./...
go vet -tags integration ./internal/config/ ./internal/database/postgres/migrations/ ./internal/api/
go test ./internal/api/ ./internal/config/ ./internal/mocks/ ./internal/server/ ./internal/analytics/ ./internal/database/postgres/migrations/ -count=1   # each package run separately, all ok
go test -race ./internal/api/ -count=1 -run 'Audit|DirectExecute|DirectExchange|DuplicateSubmitRecorded|OutcomeRecorded|GateRefusalWritesNo|FailureOutcomeClassification|RetryOfInFlight|RequestCancel'   # ok
go test -tags integration ./internal/config/ -count=1 -run TestPostgresStoreDB_RIExchangeDirectExecution   # real Postgres, 6 subtests ok
go test -tags integration ./internal/database/postgres/migrations/ -count=1 -run 'TestMigration_RIExchangeDirectExecutions|DownAll'   # ok (up, idempotent re-run, down, ri_exchange_history untouched; full down-all round trip)
go test -tags integration ./internal/api/ -count=1 -run Integration   # ok
golangci-lint v2.10.1 run --allow-parallel-runners [--build-tags integration] on the touched packages   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/ cmd/   # silent (executeExchange 9, executeAzureExchange 10, unchanged)
gosec v2.28.0 (scripts/gosec-hook.sh exclusions) ./internal/api/ ./internal/config/ ./internal/mocks/   # exit 0
git-secrets --scan in a fresh `git init` copy with isolated HOME: --register-aws (CI set) and the scripts/setup-git-secrets.sh set   # both exit 0
scripts/test-git-secrets-allowlist.sh   # 42 PASS, 0 FAIL

The migration number was checked against origin/main (highest 000130) and every open PR's files immediately before pushing. No other PR adds a migration.

Real-cloud gap. Nothing here exercised a live AWS AcceptReservedInstancesExchangeQuote or Azure exchange LRO. The failed/unknown classification relies on the existing mapAWSExchangeError/mapAzureExchangeError mapping, which this PR doesn't change.

Out of scope / follow-ups

• Reading the trail. This PR persists only. There's no API endpoint or UI for listing ri_exchange_direct_executions (the existing GET /api/ri-exchange/history reads ri_exchange_history only). Exposing it needs a scoped read endpoint (allowed_accounts filtering like test(api): pin the submit dedupe as a predicate, not a 1000-row page (closes #266) #1030) plus a frontend view, so it's left for a follow-up. Until then the trail is queryable in SQL; the idx_ri_exchange_direct_executions_unresolved partial index covers submitted/unknown rows.
• Daily spend cap. Direct executes still don't count toward the approval flow's daily cap (unchanged behavior; see the decision above).
• Gate refusals (permission, scope, 4-eyes, guardrails) are still not recorded; only requests that reach the submit stage are. A test pins this.
• handler_ri_exchange.go is already ~2850 lines, well over the 500-line guideline. The new logic lives in internal/api/ri_exchange_audit.go rather than growing it further.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, urgency/this-quarter, impact/many, effort/m, type/feat, triaged.

🤖 Generated with Claude Code

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 42a65bd4-4a86-4b69-a7cd-563aef608d63

📥 Commits

Reviewing files that changed from the base of the PR and between 8caed39 and f5b3e52.

📒 Files selected for processing: 14

• internal/analytics/collector_test.go
• internal/api/handler_ri_exchange.go
• internal/api/ri_exchange_audit.go
• internal/api/ri_exchange_audit_test.go
• internal/config/interfaces.go
• internal/config/ri_exchange_direct_execution.go
• internal/config/store_postgres_ri_exchange_direct.go
• internal/config/store_postgres_ri_exchange_direct_db_test.go
• internal/config/store_postgres_ri_exchange_direct_test.go
• internal/database/postgres/migrations/000131_ri_exchange_direct_executions.down.sql
• internal/database/postgres/migrations/000131_ri_exchange_direct_executions.up.sql
• internal/database/postgres/migrations/000131_ri_exchange_direct_executions_test.go
• internal/mocks/stores.go
• internal/server/test_helpers_test.go

+--------------------------------------------------------------------+
| I've been trying to reach you about your code's extended warranty. |
+--------------------------------------------------------------------+
  \
   \   (\__/)
       (•ㅅ•)
       / 　 づ

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review 1 (claude-fleet, cold): APPROVE f5b3e52

Findings (none blocking; low or info):

1. [low, Azure classification] directExchangeFailureStatus records "failed" ("the exchange did not happen") for any 4xx-mapped error, and isAzureClientError matches any azcore.ResponseError with status 400/403/404/405/409/422 anywhere in the chain.
   • That includes a 4xx from the poller after BeginPost was accepted. The upstream code (providers/azure/services/compute/exchange_operations.go:313-318) wraps the caller error, which covers BeginPost and PollUntilDone alike.
   • Failure scenario: a 404 or 409 on the operation poll is audited "failed" although Azure may have committed.
   • Suggest recording "unknown" for Azure errors unless they are provably from before submit. The PR relies on the existing mapping, so this can be a follow-up.
2. [low] On a claim-store error (DB down), the follow-up finalize likely fails too. The row stays "submitted" (the "outcome never recorded" signal) even though the provider was never called. This errs on the safe side and is worth a runbook line.
3. [info] auditDecimal writes the cap at full precision. A cap like 1e-20000 produces more than the 16383 fractional digits NUMERIC allows, so the audit insert fails and the exchange is refused with a 500 instead of a 400. This fails closed and only affects absurd input.
4. [info, YAGNI] Nothing reads the table yet (no reader for the indexes or JSON tags in this PR). The body states this and calls the trail SQL-queryable, so it is acceptable.
5. [info] TestMigrations_DownAllRoundTrip passes with a no-op 000131 down. Only TestMigration_RIExchangeDirectExecutions catches a broken down.
6. [by design] There is no unique or idempotency index. Each attempt gets its own row with a caller-generated UUID primary key, and dedupe stays with the #1642 claim. A rejected duplicate gets a row finalized "failed" (not submitted to the provider).

Migration numbering and order:

• The highest migration on origin/main is 000130 (purchase_executions_created_by_system). 000131 is next, with no gap and no duplicate.
• origin/main is 23 commits past the merge-base, and git merge-tree against it merges cleanly.
• The merge order is strict: feat(exchange): record an audit trail for direct-API RI exchange executes #1092 (131), then fix(analytics): cut savings_snapshots partitions at UTC month starts #1093 (132), then fix(history): clear stale convertible stamps on AWS EC2 RI history rows #1105 (133). golang-migrate applies only versions above the current one, so out of order a lower number is silently skipped on databases that are already migrated.

Up-down symmetry:

• Up is CREATE TABLE IF NOT EXISTS plus two CREATE INDEX IF NOT EXISTS, with no triggers, enums, foreign keys or changes to other tables.
• Down is DROP TABLE IF EXISTS, an exact reverse. It loses data only from the new table, as the file notes.
• Up is idempotent: a raw re-execution is tested.
• Both files are single transactional statements, and TestMigrationsAreTransactional passes.

Attacked:

• Money semantics on AWS (upstream executeWithAPI, pkg/exchange/exchange.go:408-460):
  • Guardrail refusals before Accept are recorded "unknown" with the amount.
  • A 4xx from Accept (RetryMaxAttempts=1) is correctly "failed".
  • An empty ExchangeId is "unknown".
  • The currency comes from CurrencyCode, which is forced to USD.
• Money semantics on Azure: preview is checked for nil before the audit build, and an empty currency is rejected with a 400 before any audit.
• CAS: WHERE id=$1 AND status='submitted', with RowsAffected 0 mapped to a sentinel. A CHECK enforces (status='submitted') = (completed_at IS NULL), and timestamps are TIMESTAMPTZ from now().
• Scoping:
  • There is no read path.
  • cloud_account_id comes from the server (AWS) or from the account resolved after the scope check (Azure).
  • provider_account_id is the scope-checked subscription.
• Wiring: the store is wired into both handlers, through StoreInterface and the mock.
• Mutation probes, each restored and confirmed identical with cmp:
  • The AND status='submitted' guard dropped: 3 tests FAIL, including the racing-finalize and pgxmock CAS tests.
  • The down replaced by SELECT 1: FAILS.
  • The completed_at CHECK replaced by CHECK (true): FAILS.
  • Account attribution dropped, AWS and Azure: FAILS.
  • The claim taken before the insert: 5 FAILs.
  • Unknown recorded as failed: FAILS.
  • The finalize run on the request context instead of a detached one: FAILS.
  • The AWS success finalize dropped: 3 FAILs.
• The 000131 test FAILS on the base export.

Verified locally, on a git archive export of HEAD with no cloud credentials:

• gofmt clean, go build ./... ok, and go vet ok, including -tags integration.
• go test -count=1 on api, config, mocks, server, analytics and migrations: all ok, and -race on the audit tests passes.
• -tags integration (testcontainers, under the heavy lock): TestMigration_RIExchangeDirectExecutions, TestMigrations_DownAllRoundTrip and TestPostgresStoreDB_RIExchangeDirectExecution all PASS, with all 6 subtests run.
• Not run: the api integration tests, golangci-lint and gosec.

Reviewed by an independent agent with no author context (first reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

VERDICT: APPROVE f5b3e52

Independent adversarial review of #1092 (closes #82) at head f5b3e525, base origin/main 8fa921f3 (merge-base 8caed39c, single commit).

Findings (none blocking):

• merge-order internal/database/postgres/migrations/000131_ri_exchange_direct_executions.up.sql:35 - 000131 is the next free number (origin/main tops out at 000130; fix(analytics): cut savings_snapshots partitions at UTC month starts #1093 adds 000132, fix(history): clear stale convertible stamps on AWS EC2 RI history rows #1105 adds 000133). The three PRs are not stacked. If fix(analytics): cut savings_snapshots partitions at UTC month starts #1093 or fix(history): clear stale convertible stamps on AWS EC2 RI history rows #1105 merged first and a database migrated to 132 or 133, golang-migrate would never apply 131. The table would be missing, SaveRIExchangeDirectExecution would fail, and beginDirectExchange (internal/api/ri_exchange_audit.go:47-49) would refuse every direct exchange. Merge strictly feat(exchange): record an audit trail for direct-API RI exchange executes #1092 -> fix(analytics): cut savings_snapshots partitions at UTC month starts #1093 -> fix(history): clear stale convertible stamps on AWS EC2 RI history rows #1105.
• nit internal/api/ri_exchange_audit.go:78 - AWS pre-Accept guardrail refusals from pkg/exchange.ExecuteExchange are recorded as unknown, not failed. The PR body discloses this and it is the pessimistic choice. Acceptable.
• nit - The table has no reader, API or retention yet. The PR body says this is out of scope. Rows are low volume and there are no FKs, so this is fine as a follow-up.

Up/down and locking: up only creates a new table plus two indexes, with no backfill and no lock on existing tables; uuid_generate_v4 comes from 000001. Down (DROP TABLE IF EXISTS) reverses up completely and is lossy by design. No Go code reads the new columns, so there is no NULL scan path. The writer passes NULLs explicitly (optionalString, nullStringFromString, nil *string), and the integration subtest "AWS row has no sources detail" stores NULL sources, quoted_net_payable and provider_account_id.

Attacked:

• Diff vs chore(exchange): persist an audit-trail record for manual/direct-API RI exchange executes (AWS + Azure) #82 scope: covers both handlers (handler_ri_exchange.go:1272/1295/1298 Azure, 1967/1989/1992 AWS), plus account, sources, targets, session/exchange id, net payable + currency, status, actor and timestamps.
• Azure "completed" can't hide a failed operation: the pinned provider ExecuteExchange (exchange_operations.go:321-327) returns an error for any non-accepted terminal status.
• Azure currency is non-empty: it is required at handler_ri_exchange.go:673, so Validate can't refuse a valid request.
• StoreInterface change: every implementation compiles (go build ./... and go vet -tags integration ./... are clean).
• Mutation probes (scratch copy, each reverted):
  • M1, drop AND status = 'submitted': killed (pgxmock CAS test).
  • M2, insert failure does not refuse: killed (AWS and Azure AuditInsertFailureRefusesExchange).
  • M3, ambiguous errors recorded as failed: killed (both FailureOutcomeClassification).
  • M4, finalize on the request ctx instead of a detached one: killed (OutcomeRecordedAfterRequestCancel).
  • M5, claim taken before the audit insert: killed (AuditRowWrittenBeforeClaimAndCommit and 4 others).
• Local runs on the exact export (AWS config set to /dev/null; Docker testcontainers under the heavy lock):
  • go build ./... and go vet -tags integration ./...: ok.
  • go test for api, config, mocks, server, analytics and migrations: ok.
  • Integration TestPostgresStoreDB_RIExchangeDirectExecution: ok.
  • Integration TestMigration_RIExchangeDirectExecutions, DownAllRoundTrip and FullStackIdempotent: ok.
  • Combined tree (feat(exchange): record an audit trail for direct-API RI exchange executes #1092 + the 000132 files from fix(analytics): cut savings_snapshots partitions at UTC month starts #1093) migrations integration: ok.

[cristim]

Local verification at f5b3e5252c4172972b2157d6480397302836b36d merged onto current main (5fc5eb24, after #1088, #1091, #1143, #1094 and #1085; merge-tree 5dbe285b; macOS with Docker testcontainers):

• go build ./... and go vet -tags integration ./... pass.
• go test -race -short passes for ./internal/api, ./internal/config and ./internal/server.
• go test -tags integration -run 'RIExchange|Migration' passes for ./internal/config and ./internal/database/..., including the 000131 migration up/down round-trip on real Postgres.
• Main's latest migration is 000130, so 000131 applies in order. Merge order stays feat(exchange): record an audit trail for direct-API RI exchange executes #1092, then fix(analytics): cut savings_snapshots partitions at UTC month starts #1093, then fix(history): clear stale convertible stamps on AWS EC2 RI history rows #1105.

[cristim]

Marked ready and merged by the owner's agent after 2 independent reviews and local verification (see the review and verification comments above, all at f5b3e5252c4172972b2157d6480397302836b36d).