fix(exchange): enforce daily-cap integrity on manual path, actual-paid ledger, and headroom (adversarial-review follow-ups) - #1457
Conversation
|
@coderabbitai review |
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRI exchange execution now bounds payment caps by daily headroom, persists AWS-accepted payment amounts, retries ledger completion, and halts automatic processing after unrecoverable ledger failures. Store interfaces, PostgreSQL queries, handlers, mocks, and regression tests were updated accordingly. ChangesRI exchange consistency
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RunAutoExchange
participant processRecommendation
participant AWS
participant saveLedgerRecord
RunAutoExchange->>processRecommendation: process recommendation
processRecommendation->>AWS: Execute with effective cap
AWS-->>processRecommendation: exchange ID and fresh quote
processRecommendation->>saveLedgerRecord: save accepted payment amount
saveLedgerRecord-->>processRecommendation: success or repeated failure
processRecommendation-->>RunAutoExchange: outcome and halt signal
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Action performedReview finished.
|
d78f295 to
62e4823
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
…d ledger, and headroom (adversarial-review follow-ups)
H1: checkDailyCap fails closed on unparseable paymentDue (was treating as $0).
H2: executeApprovedExchange and processAutoExchange bound Execute MaxPaymentDueUSD
by min(perExchangeCap, dailyCap-dailySpent) so re-quotes cannot breach headroom.
H3: Add CompleteRIExchangeWithPayment to persist the accepted (not pre-execution)
amount; both auto and manual paths use the fresh quote from Execute.
H4: Retry ledger write 3x after money moves; halt auto run / return 500 on
persistent failure to prevent cap bypass via missing ledger rows.
M5: GetRIExchangeDailySpend includes processing rows (COALESCE on completed_at/
updated_at) to close concurrent-approval TOCTOU window.
Each defect has a regression test that fails pre-fix and passes post-fix.
…uteHeadroom Fixes golangci-lint errcheck failure on handler_ri_exchange.go:1351 where `exchange.ParseDecimalRat` error was discarded with `_`. The fix extracts a `checkCapsAndComputeHeadroom` helper that bundles the three cap-validation checks (MaxDailyUSD==0, checkDailyCap, MaxPerExchangeUSD==0) and the headroom calculation, properly returning a reason string on any parse failure (fail closed). This also brings `executeApprovedExchange` back under gocyclo -over 10 (complexity 8) after the helper reduces it from the 12 that errcheck enforcement would have caused. Rebased onto main (after PR #1456 merged; no conflicts in store_postgres.go).
62e4823 to
0acb4ab
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/exchange/auto.go (1)
558-596: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
outcome.PaymentDueis never updated to the accepted amount.
accepted(the AWS-confirmed amount) is written intorecord.PaymentDuefor persistence, butoutcome.PaymentDue(initialized at line 484 to the stale pre-executionpaymentDueStr) is never reassigned.result.Completed/result.Failed— the values returned to callers ofRunAutoExchange— will therefore report the wrong payment amount even though the DB ledger row is correct, partially defeating the H3 fix's intent for any consumer relying on the outcome rather than the DB row.🐛 Proposed fix
accepted := acceptedAmountFromQuote(freshQ, paymentDueStr) + outcome.PaymentDue = accepted🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/exchange/auto.go` around lines 558 - 596, Update the auto-exchange completion flow after acceptedAmountFromQuote produces accepted so outcome.PaymentDue is assigned the AWS-confirmed amount before returning. Keep record.PaymentDue persistence unchanged and ensure both successful and subsequent ledger-save-failure outcomes expose the accepted value.
🧹 Nitpick comments (2)
internal/api/handler_ri_exchange.go (2)
1387-1395: 🧹 Nitpick | 🔵 TrivialConsider explicit alerting on persistent ledger-write failure.
When all 3 retries fail, real money has already moved (AWS exchange executed) but the ledger row was never updated — a financial reconciliation gap. The error is logged and propagated as a 500, but nothing here signals this needs urgent operator attention (vs. an ordinary request failure). Consider a dedicated metric/alert (e.g., a counter or paging-level log) for this specific failure mode so it can't silently get lost in general error-rate noise.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handler_ri_exchange.go` around lines 1387 - 1395, Add explicit operational alerting in the retryCompleteWithPayment failure branch for the persistent ledger-write failure after money moved. Emit a dedicated metric or paging-level log before returning the existing wrapped error, using the exchange identifier and preserving the current error response behavior.
1271-1289: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider backoff between ledger-write retries.
retryCompleteWithPaymentretriesCompleteRIExchangeWithPayment3 times back-to-back with no delay, hitting the DB immediately after each failure. A small fixed/backoff delay would reduce pressure on an already-struggling DB without materially slowing the (already money-moved) completion path.♻️ Proposed fix
func (h *Handler) retryCompleteWithPayment(ctx context.Context, id, exchangeID, acceptedPaymentDue string) error { const maxAttempts = 3 var err error for attempt := 1; attempt <= maxAttempts; attempt++ { err = h.config.CompleteRIExchangeWithPayment(ctx, id, exchangeID, acceptedPaymentDue) if err == nil { return nil } if attempt < maxAttempts { logging.Warnf("ledger write retry %d/%d for exchange %s after money moved: %v", attempt, maxAttempts, id, err) + select { + case <-time.After(time.Duration(attempt) * 200 * time.Millisecond): + case <-ctx.Done(): + return ctx.Err() + } } } return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handler_ri_exchange.go` around lines 1271 - 1289, Add a small delay with backoff between failed attempts in retryCompleteWithPayment, applying it only when another retry remains and before the next CompleteRIExchangeWithPayment call. Preserve the three-attempt limit, existing retry logging, context handling, and final error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/handler_ri_exchange.go`:
- Around line 1322-1341: Update checkCapsAndComputeHeadroom so its checkDailyCap
call uses the already aggregated daily spend without adding the current record’s
paymentDue; preserve the existing re-parse and headroom calculation logic.
In `@internal/config/store_postgres_pgxmock_test.go`:
- Around line 1411-1428: Update
TestPGXMock_GetRIExchangeDailySpend_IncludesProcessingStatus so its pgxmock
ExpectQuery pattern explicitly matches the SQL status filter containing
“processing” (and the existing completed condition as applicable), rather than
only “SELECT”. Preserve the current setup and assertions while ensuring the test
fails if the processing status predicate is removed.
---
Outside diff comments:
In `@pkg/exchange/auto.go`:
- Around line 558-596: Update the auto-exchange completion flow after
acceptedAmountFromQuote produces accepted so outcome.PaymentDue is assigned the
AWS-confirmed amount before returning. Keep record.PaymentDue persistence
unchanged and ensure both successful and subsequent ledger-save-failure outcomes
expose the accepted value.
---
Nitpick comments:
In `@internal/api/handler_ri_exchange.go`:
- Around line 1387-1395: Add explicit operational alerting in the
retryCompleteWithPayment failure branch for the persistent ledger-write failure
after money moved. Emit a dedicated metric or paging-level log before returning
the existing wrapped error, using the exchange identifier and preserving the
current error response behavior.
- Around line 1271-1289: Add a small delay with backoff between failed attempts
in retryCompleteWithPayment, applying it only when another retry remains and
before the next CompleteRIExchangeWithPayment call. Preserve the three-attempt
limit, existing retry logging, context handling, and final error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b42e517-21d1-4c7f-838b-b8c1db81a7ab
📒 Files selected for processing (13)
internal/analytics/collector_test.gointernal/api/exchange_helpers_test.gointernal/api/handler.gointernal/api/handler_ri_exchange.gointernal/api/handler_ri_exchange_test.gointernal/config/interfaces.gointernal/config/store_postgres.gointernal/config/store_postgres_nil_db_test.gointernal/config/store_postgres_pgxmock_test.gointernal/mocks/stores.gointernal/server/test_helpers_test.gopkg/exchange/auto.gopkg/exchange/auto_test.go
| func checkCapsAndComputeHeadroom(dailySpendStr, paymentDue string, cfg *config.GlobalConfig) (effectiveCap *big.Rat, reason string) { | ||
| if cfg.RIExchangeMaxDailyUSD == 0 { | ||
| return nil, "daily spending cap is not configured (RIExchangeMaxDailyUSD is 0)" | ||
| } | ||
| if reason := checkDailyCap(dailySpendStr, paymentDue, cfg.RIExchangeMaxDailyUSD); reason != "" { | ||
| return nil, reason | ||
| } | ||
| if cfg.RIExchangeMaxPerExchangeUSD == 0 { | ||
| return nil, "per-exchange spending cap is not configured (RIExchangeMaxPerExchangeUSD is 0)" | ||
| } | ||
| // checkDailyCap already verified dailySpendStr is parseable; a second failure | ||
| // is an internal error - fail closed to avoid executing with wrong headroom. | ||
| dailySpent, err := exchange.ParseDecimalRat(dailySpendStr) | ||
| if err != nil || dailySpent == nil { | ||
| return nil, fmt.Sprintf("daily spend re-parse failed (internal error): %v", err) | ||
| } | ||
| dailyCap := new(big.Rat).SetFloat64(cfg.RIExchangeMaxDailyUSD) | ||
| perExchangeCap := new(big.Rat).SetFloat64(cfg.RIExchangeMaxPerExchangeUSD) | ||
| return handlerChooseEffectiveCap(dailyCap, dailySpent, perExchangeCap), "" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether the record's own status is already 'processing' before
# executeApprovedExchange runs (i.e. before the daily-spend query).
rg -nP -B5 -A5 "executeApprovedExchange\(" --type=go internal/api
rg -nP -B3 -A15 "'processing'" --type=go internal/api internal/config
rg -nP -B3 -A10 "TransitionRIExchangeStatus\(" --type=go internal/apiRepository: LeanerCloud/CUDly
Length of output: 15528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant cap logic and nearby tests.
sed -n '1388,1435p' internal/api/handler_ri_exchange.go
printf '\n----\n'
sed -n '1316,1342p' internal/api/handler_ri_exchange.go
printf '\n----\n'
sed -n '1648,1778p' internal/api/handler_ri_exchange_test.goRepository: LeanerCloud/CUDly
Length of output: 9208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether tests/docs describe the daily spend query semantics and whether
# any code adjusts for the current record's own reserved amount.
rg -n "GetRIExchangeDailySpend|daily spend|processing rows|headroom|checkDailyCap|checkCapsAndComputeHeadroom" internal/config internal/apiRepository: LeanerCloud/CUDly
Length of output: 8274
Avoid double-counting the in-flight exchange in checkDailyCap (internal/api/handler_ri_exchange.go:1403-1421)
GetRIExchangeDailySpend already includes processing rows, and this record is moved to processing before executeApprovedExchange calls the cap check. Adding record.PaymentDue again makes the gate stricter than intended and can reject valid approvals near the cap. The headroom calculation below is fine; the preflight check is the part that needs to change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/api/handler_ri_exchange.go` around lines 1322 - 1341, Update
checkCapsAndComputeHeadroom so its checkDailyCap call uses the already
aggregated daily spend without adding the current record’s paymentDue; preserve
the existing re-parse and headroom calculation logic.
| // TestPGXMock_GetRIExchangeDailySpend_IncludesProcessingStatus verifies that | ||
| // the query includes 'processing' rows in addition to 'completed' ones (M5 | ||
| // fix) by asserting the SQL contains the expected status filter text. | ||
| // Because pgxmock matches the full query string, we confirm the SQL sent to | ||
| // the DB driver contains "processing". | ||
| func TestPGXMock_GetRIExchangeDailySpend_IncludesProcessingStatus(t *testing.T) { | ||
| mock := newMock(t) | ||
| store := storeWith(mock) | ||
| ctx := context.Background() | ||
|
|
||
| // Match any SELECT that goes to the DB; the important assertion is below. | ||
| rows := pgxmock.NewRows([]string{"total"}).AddRow("0") | ||
| mock.ExpectQuery("SELECT").WithArgs(pgxmock.AnyArg()).WillReturnRows(rows) | ||
|
|
||
| _, err := store.GetRIExchangeDailySpend(ctx, time.Now()) | ||
| require.NoError(t, err) | ||
| assert.NoError(t, mock.ExpectationsWereMet()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test doesn't actually verify the "processing" status filter.
The docstring/comment says this confirms the SQL contains "processing", but mock.ExpectQuery("SELECT") only regex-matches on "SELECT". This test would pass unchanged even if the M5 fix (including 'processing' rows) were reverted, so it provides no regression protection for the defect it's named after.
🧪 Proposed fix to actually assert the status filter
- // Match any SELECT that goes to the DB; the important assertion is below.
- rows := pgxmock.NewRows([]string{"total"}).AddRow("0")
- mock.ExpectQuery("SELECT").WithArgs(pgxmock.AnyArg()).WillReturnRows(rows)
+ rows := pgxmock.NewRows([]string{"total"}).AddRow("0")
+ mock.ExpectQuery(`(?s)SELECT.*status IN \('completed', 'processing'\)`).
+ WithArgs(pgxmock.AnyArg()).WillReturnRows(rows)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TestPGXMock_GetRIExchangeDailySpend_IncludesProcessingStatus verifies that | |
| // the query includes 'processing' rows in addition to 'completed' ones (M5 | |
| // fix) by asserting the SQL contains the expected status filter text. | |
| // Because pgxmock matches the full query string, we confirm the SQL sent to | |
| // the DB driver contains "processing". | |
| func TestPGXMock_GetRIExchangeDailySpend_IncludesProcessingStatus(t *testing.T) { | |
| mock := newMock(t) | |
| store := storeWith(mock) | |
| ctx := context.Background() | |
| // Match any SELECT that goes to the DB; the important assertion is below. | |
| rows := pgxmock.NewRows([]string{"total"}).AddRow("0") | |
| mock.ExpectQuery("SELECT").WithArgs(pgxmock.AnyArg()).WillReturnRows(rows) | |
| _, err := store.GetRIExchangeDailySpend(ctx, time.Now()) | |
| require.NoError(t, err) | |
| assert.NoError(t, mock.ExpectationsWereMet()) | |
| } | |
| func TestPGXMock_GetRIExchangeDailySpend_IncludesProcessingStatus(t *testing.T) { | |
| mock := newMock(t) | |
| store := storeWith(mock) | |
| ctx := context.Background() | |
| rows := pgxmock.NewRows([]string{"total"}).AddRow("0") | |
| mock.ExpectQuery(`(?s)SELECT.*status IN \('completed', 'processing'\)`). | |
| WithArgs(pgxmock.AnyArg()).WillReturnRows(rows) | |
| _, err := store.GetRIExchangeDailySpend(ctx, time.Now()) | |
| require.NoError(t, err) | |
| assert.NoError(t, mock.ExpectationsWereMet()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/config/store_postgres_pgxmock_test.go` around lines 1411 - 1428,
Update TestPGXMock_GetRIExchangeDailySpend_IncludesProcessingStatus so its
pgxmock ExpectQuery pattern explicitly matches the SQL status filter containing
“processing” (and the existing completed condition as applicable), rather than
only “SELECT”. Preserve the current setup and assertions while ensuring the test
fails if the processing status predicate is removed.
Summary
Five verified money-integrity defects in the RI Exchange daily-cap subsystem, fixed with regression tests.
checkDailyCapinhandler_ri_exchange.gowas treating an unparseablepaymentDueas $0 and proceeding. Now fails closed with a blocking reason (mirrors the existingdailySpentparse-failure path).executeApprovedExchangeandprocessAutoExchangepassed the fullperExchangeCapasMaxPaymentDueUSDtoExecute. A fresh re-quote could accept an amount that exceeds remaining daily headroom. Fixed:effectiveCap = min(perExchangeCap, dailyCap - dailySpent).Executewas discarded (_, _). Completed records persisted the stale pre-execution quote.GetRIExchangeDailySpendsums ledger amounts, so the daily spend was systematically under-counted. Fixed: newCompleteRIExchangeWithPaymentmethod persists the accepted amount; both paths use it.CompleteRIExchange/SaveRIExchangeRecordwrites after money moved were log-and-continue. The daily ledger would under-count, allowing subsequent exchanges to bypass the cap. Fixed: 3-attempt retry; auto run halts / handler returns HTTP 500 on persistent failure.GetRIExchangeDailySpendonly summedstatus = 'completed'rows. Concurrent approvals both read the same spend total before either write commits (TOCTOU). Fixed: SQL now includes'processing'rows viaCOALESCE(completed_at, updated_at)date filter.Test plan
TestCheckDailyCap_UnparseablePaymentDue_FailsClosed,TestProcessAutoExchange_EffectiveCap_BoundedByDailyHeadroom,TestProcessAutoExchange_AcceptedAmountFromFreshQuote,TestProcessAutoExchange_LedgerSaveFailure_HaltsAndReturnsError,TestRunAutoExchange_LedgerSaveFailure_StopsAfterFirstExchange,TestExecuteApprovedExchange_EffectiveCap_BoundedByDailyHeadroom,TestExecuteApprovedExchange_AcceptedAmountFromFreshQuote,TestExecuteApprovedExchange_LedgerWriteFailure_ReturnsError)Summary by CodeRabbit