[cristim]

Summary

Closes #1829. The direct AWS RI exchange execute path bypassed #1826's source-in-flight guard, in both directions:

• Direct after unknown: a direct execute could exchange a source RI that an approve or AUTO exchange held in an unknown (processing) state.
• Unknown after direct: a direct execute whose outcome was unknown never blocked a later approve, AUTO or direct exchange of the same RI.

Either way, two exchanges of one source RI could both reach AWS.

The design follows #1829's owner decisions (D1 = D2 = D3 = A):

• The guard reads the existing direct-execution audit table, for AWS rows that are submitted or unknown, under the same advisory spend lock that approve and AUTO already take.
• No migration and no synthesized history rows.
• fix(ri-exchange): keep an exchange with a lost accept response in flight #1826's guard body is unchanged.
• A refused direct request returns 409 and writes nothing (D3).
• Azure is out of scope (D2).
• The guard ships before a resolve API exists, with a runbook as the interim (D1). The resolve API is feat(ri-exchange): list and resolve stuck direct-execution rows (follow-up to #1829) #1836.

Commits (each one builds; all at or under 400 lines)

1. unknown only when the outcome is unknown (T0). An AWS direct execute is recorded unknown only when the library marks the error ErrOutcomeUnknown, otherwise failed.
   • classifyAcceptError (cloud-commitments-go pkg/exchange/outcome.go) wraps every accept error except a definite 4xx or a throttle.
   • Pre-accept errors (payment over max, invalid quote, STS, throttles) are definite rejects.
   • Before this, any non-4xx was unknown, and with the guard that would have blocked RIs on ordinary user errors.
2. Reservations honour unresolved direct rows (T1). A sibling guard, refuseRIExchangeSourceInFlightDirect, runs next to fix(ri-exchange): keep an exchange with a lost accept response in flight #1826's in ReserveRIExchange and ReserveApprovedRIExchange, in the same transaction after the lock.
   • It normalises IDs itself: lowercase, trimmed of ASCII whitespace, compared via btrim(x, E' \t\n\r\f\x0B').
   • It fails closed on a lookup error.
   • Its error names only the caller's shared RI IDs. The blocking row's ID goes to the server log only (sec(api): RI exchange session approve still leaks missing (404) vs not-pending (409) to approve-own callers #1428).
3. History shows the new refusal through a closed errnotes pattern. The approve path stores it.
4. The direct save is guarded and locked (T2). SaveRIExchangeDirectExecution is one transaction: AWS takes the shared spend lock (lockRIExchangeSpend, now shared with reserveRIExchangeCeiling), then runs both guards, then inserts with created_at = clock_timestamp(), taken after the lock.
5. The handler returns 409 before the claim and the AWS call (T3). A refusal leaves no audit row, no idempotency claim and no AWS call.
6. Runbook and known issues (T4). runbooks/ri-exchange-direct-execution-settle.md is the interim operator step: find the row, verify at AWS, then the guarded settle SQL.
   • The SQL refuses submitted rows younger than 5 minutes (the execution window is 4 minutes).
   • It refuses Azure rows and already-settled rows, and appends to error rather than overwriting it.
   • An integration test runs that exact SQL, read from the runbook file.

Behaviour changes

• POST direct AWS execute can now return 409 when a source RI is held by an in-flight approve or AUTO exchange, or by an unresolved direct execution. openapi is updated. The envelope is the existing error envelope.
• A failed AWS direct execute is now failed, not unknown, unless the accept outcome is genuinely unknown. Migration 000131's comment wording is stale for AWS; the runbook says so.

Known issues (in known-issues.md)

• Direct executes stay outside the daily spend cap. That's a deliberate design choice (migration 000131), and the risk is noted.
• Until feat(ri-exchange): list and resolve stuck direct-execution rows (follow-up to #1829) #1836, settling an unresolved direct row is a DBA step (the runbook).
• History IDs come only from AWS describe calls, which are lowercase. A non-lowercase stored history ID would match only identical input; fix(ri-exchange): keep an exchange with a lost accept response in flight #1826's guard compares exactly.

Verification

• Each task, test-first: red on the base, green after.
• Mutation probes: each fails a named test. Dropping either guard call, the lock, the provider, status or overlap filters, the Go or SQL normalisation, the 409 mapping, clock_timestamp(), the runbook age or provider clause, or the errnotes pattern; leaking the blocker ID; running the executor or the claim before the save. A trim set using \v, which PostgreSQL reads as the letter v, is also caught by a dedicated test.
• Concurrency: a lock proof via AwaitBlockedBy, and a race test (6 parallel saves, -count=20, exactly one wins).
• Integration: the issue's repro (409, 0 AWS calls, 0 audit rows, 0 idempotency rows) and the reverse (502 unknown; then AUTO and approve refused; then a disjoint RI reserves).
• Reviews:
  • plan: 10 adversarial passes, closing with 3 clean in a row as the platform requires;
  • per task: 2 passes each;
  • full change: 3 passes, closed on agreement.

Lock ordering: the only row lock taken before the advisory lock is ReserveApprovedRIExchange's own FOR UPDATE. Every other advisory-lock holder takes no other row lock, and the claim and Finalize run outside the locked transaction, so there's no cycle.

Note for whichever of this and #1831 merges second: directGuardIDListPattern duplicates #1831's riIDListPattern, so reuse that one.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 16 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/1829-direct-execute-inflight-guard
• File: .coderabbit.yaml
• Commit: 57f0711
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: f6d16c73-b678-403b-b21a-aa828d81d425

Commits

Reviewing files that changed from the base of the PR and between 8bdc4d9 and 57f0711.

Files selected for processing: 21

• internal/api/handler_ri_exchange.go
• internal/api/handler_ri_exchange_direct_guard_integration_test.go
• internal/api/handler_ri_exchange_direct_guard_test.go
• internal/api/openapi.yaml
• internal/api/ri_exchange_audit.go
• internal/api/ri_exchange_audit_test.go
• internal/api/ri_exchange_direct_outcome_unknown_test.go
• internal/api/ri_exchange_refusals.go
• internal/config/ri_exchange_direct_execution.go
• internal/config/store_postgres.go
• internal/config/store_postgres_pgxmock_test.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_guard_db_test.go
• internal/config/store_postgres_ri_exchange_direct_guard_integration_test.go
• internal/config/store_postgres_ri_exchange_direct_runbook_integration_test.go
• internal/config/store_postgres_ri_exchange_direct_test.go
• internal/purchase/errnotes.go
• internal/purchase/errnotes_test.go
• known-issues.md
• runbooks/ri-exchange-direct-execution-settle.md

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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

APPROVED f80af7a by claude-ui-fix-1

I did the cold review at 2933fdc. The head then moved to f80af7a with one comment-only commit: internal/config/store_postgres.go:3529, "serialises" → "serializes". git diff --stat shows 1 file, +1/-1, so the review carries over unchanged.

Findings (none block; I tried to refute each one first)

1. Low, doc. runbooks/ri-exchange-direct-execution-settle.md (Step 2 and Step 3 SQL) uses bare :ri, :id, :new_status and :note. In psql those need :'id'-style quoting. As written, the statement fails with a syntax error, which is safe. The integration test substitutes $n, so it doesn't catch this. Fix: use :'x', or say the values must be quoted.
2. Info. The 409 text says an operator must verify the outcome at AWS and settle the record, without naming the runbook. The openapi and known-issues.md both point to the runbook, so this is fine.
3. Info. directGuardIDListPattern (internal/purchase/errnotes.go:105) duplicates riIDListPattern from fix(purchase): show the RI exchange guard and outcome notes in History #1831. As the PR body says, whichever PR merges second should reuse the other's.

What I attacked

• TOCTOU. In store_postgres_ri_exchange_direct.go:40-49, one transaction runs lockRIExchangeSpend, then both guards, then the INSERT, then the commit.
  • It takes the same global key as fix(ri-exchange): keep an exchange with a lost accept response in flight #1826's reserveRIExchangeCeiling.
  • The submitted row commits before the AWS call, so later guards running at READ COMMITTED see it.
  • Reserve and ReserveApproved run the new sibling guard (lines 3362 and 3413) after the lock.
  • No deadlock cycle: only ReserveApproved takes a row lock before the advisory lock, and Save takes no history-row locks.
• Status classification (a wrong failed would release the RI).
  • Every accept error is wrapped as outcome-unknown except a definite 4xx or a throttle; I checked the pinned cloud-commitments-go exchange/outcome.go:21-35.
  • So awsDirectFailureStatus (ri_exchange_audit.go:215) marks failed only on a definite AWS rejection.
• Permanent block.
  • "In flight" means AWS rows in submitted or unknown.
  • The runbook's settle SQL refuses Azure rows, rows that are already settled, and submitted rows younger than 5 minutes (the timeout is 4 minutes). It appends to error, and an integration test executes the runbook SQL.
  • created_at = clock_timestamp() keeps the lock wait from aging a row.
• The 409 writes nothing. ri_exchange_audit.go:49-52 returns before the claim and the executor, and the transaction rolls back. The body names only the caller's shared RI IDs; the blocking row's ID goes to the server log only.
• Azure. The lock and guard sit behind record.Provider == "aws", and the guard SQL filters provider = 'aws'. The Azure path is unchanged.

Tests (local, macOS, testcontainers, throttled)

• Build and vet pass.
• Integration ./internal/purchase/ ./internal/api/ -run 'Exchange|RIExchange|InFlight' passes.
• Integration ./internal/config/ -run 'RIExchangeDirect|DirectGuard|InFlight|RIExchange' passes.
• TestRIExchangeDirectSave_RaceExactlyOneWins -count=10 passes.

Mutation probes (each reverted)

• Guard dropped: fails TestRIExchangeDirectSave_GuardRefusesHeldRIs, _GuardNormalisesIDs, _RaceExactlyOneWins and TestExecuteExchange_DirectGuard_Repro.
• Status filter inverted: fails 9 config tests, including the runbook test, and TestExecuteExchange_DirectGuard_Reverse.
• Guard read before the lock: fails TestPGXMock_SaveRIExchangeDirectExecution_AWSLocksAndGuardsBeforeInsert, and the race test fails 10 out of 10 runs.
• Row inserted on refusal: fails _GuardRefusesHeldRIs, _RaceExactlyOneWins and _DirectGuard_Repro.

CI. At 2933fdc, Lint Code failed only on the misspelling this head fixes. The merge needs CI green at f80af7a.

Gaps. I did no real-account AWS verification, and none is authorized.

[cristim]

APPROVED 57f0711 by claude-ui-fix-1

This is a delta over my approval at f80af7a: one commit, 2 files, +11/-10, docs and test only.

• Runbook placeholders: runbooks/ri-exchange-direct-execution-settle.md now uses psql's quoted placeholders (:'ri', :'id', :'new_status', :'note') and shows how to set them with psql -v ... or \set. That closes my Low. WHERE id = :'id' produces an untyped string literal that Postgres casts to the uuid column, and :'new_status' becomes a quoted string, so the statements are valid as written.
• Runbook test: the integration test now extracts the block that starts with the quoted form, and maps each quoted placeholder to a typed positional parameter ($1::text, $2::uuid, $3::text). The tested SQL still comes verbatim from the runbook file, so the operator's copy and the tested statement can't drift.
• Production code: no change since f80af7a.

The merge needs CI green at this SHA.

[cristim]

APPROVED 57f0711 by claude-qa-ui-4

Cold review at 2933fdc. The two commits to 57f0711 change no production code: the "serializes" comment fix at internal/config/store_postgres.go:3529 (it fixes the Lint Code misspell failure at 2933fdc), the runbook SQL switched to psql :'var' quoting, and the runbook integration test reading those placeholders. TestRIExchangeDirectRunbook, Save and DirectGuard pass at 57f0711.

• No TOCTOU: direct Save is one WithTx (ri_exchange direct.go:40): lockRIExchangeSpend (:42) takes the same global pg_advisory_xact_lock as fix(ri-exchange): keep an exchange with a lost accept response in flight #1826 (riExchangeDailySpendLockKey, store_postgres.go:3530), then both guards run (:45), then the INSERT. The lock is per deployment, so there is no per-source ordering, and it is tx-scoped. refuseRIExchangeSourcesInFlight (store_postgres.go:3465) runs after that lock in ReserveRIExchange (:3362), ReserveApprovedRIExchange (:3413) and direct Save, so direct, approve, auto/scheduled and ladder reshape all see each other's in-flight rows.
• A 409 writes nothing: the guard error rolls back the tx (no audit row), and beginDirectExchange answers 409 before claimExchangeSubmit (ri_exchange_audit.go:49, :56), so there is no claim and no AWS call. The text names only the caller's RI ids; the blocker id is logged only (store_postgres.go:3519-3520).
• Azure: its execute handler is unchanged, and the guard SQL filters provider='aws' (:3508).
• Stuck rows: no TTL or reaper touches ri_exchange_direct_executions. Until feat(ri-exchange): list and resolve stuck direct-execution rows (follow-up to #1829) #1836 lands, runbooks/ri-exchange-direct-execution-settle.md (verify at AWS, then a guarded UPDATE that refuses submitted rows under 5 minutes old, Azure rows and settled rows) is the way out, listed in known-issues.md. An integration test runs that SQL.

Runs at 2933fdc (GOMAXPROCS=4, -p 2):

• Build and vet pass; unit api/config/purchase pass.
• Integration: purchase and config full, api scoped to Exchange|RI|InFlight|Approve, all pass. The race test passes 20 of 20 runs.
• Probes, 8 of 8 killed:
  • Save skipping the guards;
  • the direct guard dropped from the combined guard;
  • no lock;
  • the guard reading before the lock;
  • 'unknown' dropped from the in-flight set;
  • an audit row written on a 409;
  • no 409 mapping;
  • a lost accept recorded as failed.

CI run 42331019594 at 57f0711 was still running when posted (28 pass, 9 pending including Lint Code). The merge needs it green.

Non-blocking:

• The runbook's 5-minute settle margin counts from created_at, but the idempotency claim runs after the insert (ri_exchange_audit.go:56) on the request context with no explicit bound, so a slow claim shortens the margin. Step 1, verifying at AWS, covers it.
• The spend lock relies on the server's default READ COMMITTED; other lock helpers set it explicitly (store_postgres_ladder_lock.go:73).
• The history guard compares RI ids exactly. This is documented, and AWS ids are lowercase.
• The entries are in the root known-issues.md, while CLAUDE.md names known_issues/.