[cristim]

Summary

Approved (manual) RI exchanges now reserve their full execution ceiling atomically under the same global advisory lock as the AUTO reservation, and in-flight reservations keep counting across UTC midnight.

Closes #656
Closes #657

What changed

• PostgresStore.ReserveApprovedRIExchange(id, dailyCap, perCap): under pg_advisory_xact_lock(riExchangeDailySpendLockKey), reads the spend with the row's own id excluded, computes ceiling = min(perCap, dailyCap - spend), refuses when the initial quote does not fit, and stores the ceiling as the row's payment_due. Shares reserveRIExchangeCeiling with ReserveRIExchange (AUTO), so both paths use one protocol.
• executeApprovedExchange no longer reads daily spend separately; it reserves, runs Execute with the reserved ceiling as MaxPaymentDueUSD, and settles the fresh accepted amount on the same row via CompleteRIExchangeWithPayment. Region and cap-config checks still run first and do not reserve.
• riExchangeDailySpendQuery: every processing row counts regardless of day (an unsettled hold), completed rows count on their completed_at UTC day. Optional $2 excludes one row.
• Removed checkDailyCap, checkCapsAndComputeHeadroom and handlerChooseEffectiveCap (replaced by the store logic) and their unit tests.

Evidence (real PostgreSQL 16, local image, no purchases, no cloud calls)

go test -tags integration ./internal/config -run 'PostgresStoreDB_(Reserve|DailySpend)', 8 tests pass:

• two concurrent approvals (initial $100 each, cap headroom $1000, per-cap $800) reserve ceilings summing exactly $1000; a later approval is refused
• legitimate $600 approval under a $1000 cap reserves $1000 (was rejected for double-counting itself)
• AUTO reservation and approval racing divide the headroom, not double it
• frozen day-B read counts a day-A unsettled $1000 hold and not the completed day-A $500
• exact UTC boundary: completed at 23:59:59.999999 counts for day A only, at 00:00:00 for day B only
• a 30h-old processing hold still reduces headroom for a new AUTO reservation
• existing lock-wait probe still passes

Non-vacuity (mutations, then restored): reverting to the day-windowed query fails the rollover tests; removing the self-exclusion fails the $600 and concurrent-approval tests. Note: the table has a BEFORE UPDATE trigger that rewrites updated_at, so the tests back-date it with session_replication_role = replica (superuser, dedicated issue41_ test DB only).

Unit: go test ./internal/{config,api,server,analytics,mocks} pass (non-race).

Notes for the reviewer

• A processing row that is never completed or failed (crash mid-exchange) now holds headroom indefinitely, which is the conservative reading of "until reconciled". There is no reconciler for stale processing rows today; tracked as a follow-up.
• If Execute returns no fresh quote amount the ledger now records the reserved ceiling (previously the initial quote). That over-counts, which fails safe.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 13 billable files and costs up to $3.25.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 9 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 82 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 48160377-6b93-4417-b679-10bd5a530e5f

---

📥 Commits

Reviewing files that changed from the base of the PR and between 30d6c84 and 6bafb61.

---

📒 Files selected for processing (13)

• internal/analytics/collector_test.go
• internal/api/exchange_helpers_test.go
• internal/api/handler_ri_exchange.go
• internal/api/handler_ri_exchange_test.go
• internal/api/middleware_test.go
• internal/api/ri_exchange_approve_reject_status_test.go
• internal/config/interfaces.go
• internal/config/store_postgres.go
• internal/config/store_postgres_pgxmock_test.go
• internal/config/store_postgres_ri_exchange_approved_reservation_integration_test.go
• internal/mocks/stores.go
• internal/server/handler_ri_exchange_test.go
• internal/server/test_helpers_test.go

---

---

• 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 of PR 690 at 9e81e93

Verdict: the money logic is correct; CHANGES REQUESTED on two small items before merge (R1 test strength, R2 Lint Code). Not approved until both are closed or explained.

What I checked and found sound

• Race for the last headroom: ReserveApprovedRIExchange row-locks its own row, then takes the same pg_advisory_xact_lock as AUTO inside the shared reserveRIExchangeCeiling, reads spend with the row's own amount excluded, and writes payment_due=ceiling in the same transaction before the lock releases. The next waiter's fresh READ COMMITTED statement sees it. Lock order is row then advisory in every approval; AUTO inserts a new row, so no deadlock cycle exists.
• Rollover: the spend query now counts every processing row regardless of day, and completed rows by completed_at inside the requested UTC day. A hold taken at 23:59:59 still reduces headroom at 00:00:00, and a row that completes after midnight is counted on the completion day. Conservative on both sides.
• Release: every refusal path (cap, parse error, reservation error, execute error) goes through failExchange to status failed, which the query no longer counts. If FailRIExchange itself fails, or the process dies mid-execute, the row stays processing and keeps holding headroom (fails safe, see R4).
• Stale quote: the handler passes the reserved ceiling (min of per-exchange cap and remaining headroom, never below the initial quote) as MaxPaymentDueUSD to exchange.ExecuteExchange. That the library enforces the fresh quote against it is in cloud-commitments-go, which I did not read; the handler test uses a stub. Unverified here.
• Other consumers: AUTO's ReserveRIExchange shares the helper; GetRIExchangeDailySpend passes a nil exclusion. No remaining caller of the removed checkDailyCap/handlerChooseEffectiveCap.

Evidence reproduced (macOS, exact SHA in the author's worktree, non-race, under the platform test-suite lock, 9 GiB free)

• Own database issue41_review690 created in the author's local postgres:16 container (additive, nothing deleted). The 7 new integration tests pass.
• go test ./internal/api ./internal/server ./internal/config -run Exchange: ok (all three).
• Mutations via go -overlay (no source touched): (a) windowing the processing rows again -> InFlightReservationSurvivesUTCRollover and PriorDayInFlightHold fail; (b) dropping the self-exclusion -> ConcurrentApprovalsStayWithinCap and DoesNotCountOwnInitialQuote fail; (c) removing the advisory lock -> only the AUTO test ReserveRIExchange_SerializesIndependentStores fails.
• Evidence is real Postgres but mocked AWS; no live cloud calls.

Findings

• R1 (should fix): the new approval-vs-approval race test does not detect removal of the lock. With the advisory lock removed, TestPostgresStoreDB_ReserveApprovedRIExchange_ConcurrentApprovalsStayWithinCap passed 15 of 15 runs (-count=15), because the two goroutines rarely overlap. The lock is only covered through the AUTO test, which happens to share the helper. Make the interleave deterministic (for example hold the advisory lock from a third transaction, start both approvals, assert both are blocked, then release) or assert on pg_locks.
• R2 (must resolve): Lint Code is failing on this head (step "Run golangci-lint", run 37906820382, still in progress so I could not read the log; golangci-lint locally was blocked by a parallel run). Judge it against main's advisory-wave failure: if it is the PR's own finding (gocyclo/func length on executeApprovedExchange or reserve helpers), it needs a fix.
• R3 (P3): when the library gives no accepted amount, the ledger now books the full reserved ceiling (up to the per-exchange cap) instead of the original quote. Over-counts spend, fails safe; state it in the PR body.
• R4 (P2, follow-up fix(exchange): reconcile stale processing RI exchange rows that now hold daily headroom indefinitely #692, agree it need not block): processing rows hold headroom indefinitely. The store already has GetStaleProcessingExchanges and the AUTO adapter exposes it, so "no reconciler exists" in fix(exchange): reconcile stale processing RI exchange rows that now hold daily headroom indefinitely #692 may be wrong or incomplete; please check whether cloud-commitments-go uses it before sizing fix(exchange): reconcile stale processing RI exchange rows that now hold daily headroom indefinitely #692.
• R5 (P3): failExchange now returns raw reservation errors ("daily spending cap check failed: failed to lock ...") in the approve response, which an email-token approver sees. Prefer a fixed message and log the detail.

[cristim]

Delta pushed: 9e81e93..754a888, addressing review 9e81e93.

• R1: the approval race test now holds the global advisory lock from a third transaction, requires both approvals to be parked on it (pg_stat_activity), asserts neither reserved, then releases. Passes 5/5 with the lock; with the lock removed from reserveRIExchangeCeiling it fails 3/3.
• R2: Lint Code log was unreadable while the run was in progress; I will fix from the log of the 754a888 run if it still fails.
• R3: stated in the PR body notes. With no accepted amount from the library, the ledger books the reserved ceiling (over-counts, fails safe).
• R4: agreed it does not block. Correction to fix(exchange): reconcile stale processing RI exchange rows that now hold daily headroom indefinitely #692: pkg/exchange/auto.go only logs a warning for processing rows older than 15 minutes; nothing settles or releases them. Issue text updated.
• R5: config.ErrRIExchangeDailyCapExceeded sentinel; cap refusals still show their numbers, any other reservation error is logged and the approver sees the fixed text "daily spending cap check failed". New unit test added.

Unit tests for config and api passed locally before the push; no further builds until disk recovers.

[cristim]

Delta review of PR 690: 9e81e93 -> f68b75b

Verdict: R1, R2 and R5 are closed. APPROVE at f68b75b, conditional on green CI on this SHA (all checks were still running when I wrote this). The 9e81e93 full-PR review otherwise stands.

• R1 closed (reproduced). The reworked ConcurrentApprovalsStayWithinCap holds the global advisory lock from a third transaction, polls pg_stat_activity until both approvals are parked on it, asserts neither has reserved yet, then releases. Unmutated: passes 10 of 10 runs (-count=10). With the advisory lock removed from the reservation path (go -overlay, no source touched): fails 5 of 5 runs with "both approvals must be parked on the global advisory lock at the same time". The earlier version passed 15 of 15 with the lock removed.
• R2 closed. Production code is untouched between 754a888 and f68b75b (git diff 754a888..f68b75b is one test file, 12+/12-: the unparam and shadow fixes). The only production change since my 9e81e93 review is the R5 delta in 754a888.
• R5 closed. executeApprovedExchange now returns the specific cap message only for errors.Is(err, ErrRIExchangeDailyCapExceeded) and a fixed "daily spending cap check failed" for everything else (details stay in the log). The sentinel is wrapped with %w, and the message text still contains "daily cap exceeded", so existing assertions hold.
• Re-run at f68b75b (non-race, under the platform test-suite lock, real PG16 in my own database, mocked AWS): the 7 reservation/spend integration tests plus the earlier ones pass; go test ./internal/api ./internal/server ./internal/config -run Exchange: ok.
• Nit (P3, optional): the waiter query reads pg_stat_activity cluster-wide. Add AND datname = current_database() so another database's advisory-lock waiter in a shared container cannot satisfy or distort the count.
• Still open and non-blocking: R3 (ledger books the full ceiling when AWS returns no accepted amount; say so in the PR body) and R4 (fix(exchange): reconcile stale processing RI exchange rows that now hold daily headroom indefinitely #692; check whether the exchange library already uses GetStaleProcessingExchanges).

[cristim]

Delta check: f68b75b -> e543fb8

Verdict: APPROVE at e543fb8 (my f68b75b approval carries over), still conditional on green CI on this SHA.

• Test-only: git diff f68b75b..e543fb8 is one file, the new integration test (7+/2-): a testAccountID helper (fmt.Sprintf("%012d", n)) replaces two AccountID: "<12 digits>" literals. No production code touched.
• Not a real secret: the flagged values were 840000000002 and 870000000002, synthetic sequential test ids from the same 8x0000000nn series used for every row in this file. Not a real AWS account id and not a credential; the helper renders the identical strings, so test behaviour is unchanged. (Eight other 12-digit string literals remain in the file as env.processing(...) arguments; they are the same synthetic series, and CI's pre-commit job is the judge of whether git-secrets accepts them.)
• Race test not weakened (reproduced at this SHA, real PG16, non-race, test-suite lock): ConcurrentApprovalsStayWithinCap passes 5 of 5 unmutated and fails 3 of 3 with the advisory lock removed (go -overlay). The 7 reservation/spend integration tests pass.
• The plan to merge main (fix(deps): bump Go to 1.26.9 and x/net to v0.60.0 for govulncheck advisories #693) into the branch and re-run the gates restarts the review at that merge SHA only if production code or conflicts change; a clean merge of test-only deltas does not need another full pass from me, but send the SHA if you want a re-ack.