[IAmJSD]

Root cause

Since #690, riExchangeDailySpendQuery counts every processing row regardless of day, so an in-flight reservation survives UTC midnight. When a run dies between the provider call and CompleteRIExchangeWithPayment/FailRIExchange (Lambda timeout, crash, or the AUTO path's unknown-outcome halt), the row stays processing and its reserved ceiling counts against the daily cap on every later day. Nothing reconciled it: RunAutoExchange only logs a WARN for rows older than 15 minutes, and there was no way to clear one short of hand-editing the DB.

Fix

Fail-safe throughout: nothing automatic ever releases a hold, because CUDly cannot tell from its own data whether the exchange happened at AWS.

1. Alarm (internal/server/handler_ri_exchange.go). The scheduled ri_exchange_reshape run now logs an ERROR line for every processing row unchanged for over 1h. ERROR is the same channel the existing "ledger write failed after money moved" errors use. Each line names the record, account, region, source RIs, held amount, how long it has been processing, and the resolve endpoint. The alarm runs before the RIExchangeEnabled check, because a stranded hold still blocks approvals of pending exchanges while automation is off. The query is origin-agnostic, so ladder-created rows are covered too. It never settles or releases anything.

2. Operator resolve: POST /api/ri-exchange/resolve/{id}, AuthUser, so the middleware enforces session and CSRF.

   • Needs execute:ri-exchange, the same deployment-account scope as approvals (out of scope returns 404), and the permission's Constraints (account, aws, ec2, checked_region), as direct execution does.
   • The stateless admin API key is refused because the decision is stamped as transitioned_by.
   • Body: outcome (completed | failed), reason (10–300 chars), an optional exchange_id (completed only), and a required cloud_check {checked_account_id, checked_region, checked_at}. failed also requires acknowledge_unknown_outcome: true.
   • completed keeps payment_due (the reserved ceiling, an upper bound of what AWS accepted, since Execute ran with it as MaxPaymentDueUSD) and stamps completed_at. The row then counts on today's UTC day only, like any settled exchange.
   • failed releases the hold.

3. Store (PostgresStore.ResolveStaleRIExchange, new file internal/config/store_postgres_ri_exchange_resolve.go). It takes SELECT … FOR UPDATE on the row and refuses unless all of these hold:

   • the row is still processing, so a row a late writer settled is never overwritten;
   • it has been unchanged for RIExchangeResolveStaleAfter = 6h, measured on the database clock. That is well past every writer's budget (4m30s on the API path, the 15m default task timeout on the scheduled path), and matches LadderFiredStaleAfter;
   • for a release, no provider exchange_id is recorded (AWS said it happened);
   • the cloud check names the row's own account and region, and its checked_at falls after the row's last change and is not in the future.

   The UPDATE repeats AND status = 'processing'. An identical retry replays (200, replayed: true). Any other request against a resolved row gets 409. No migration: the decision goes in the existing error, transitioned_by and transitioned_at columns.

Concurrency. The plausible races are a late CompleteRIExchangeWithPayment against an operator release, and two operators at once. Both serialize on the row lock.

• Late settlement vs. release. A release queued behind a late settlement sees completed and refuses. A release that locks first is overwritten by the queued settlement, because that UPDATE is unguarded. Either way the exchange ends up counted.
• Two operators. Exactly one decision lands.

Both races are tested on real Postgres with deterministic lock ordering, as listed below. The staleness window means neither race should happen in practice.

Verification

All local. No AWS calls (the handler's account resolver is injected, and runs used AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid), no purchases, no email.

Real PostgreSQL 16 (postgres:16-alpine, dedicated issue41_ DB, migrations to 130):

RI_EXCHANGE_TEST_DATABASE_URL=postgres://…/issue41_i692 \
  go test -tags integration ./internal/config -run 'PostgresStoreDB_(Resolve|Reserve|DailySpend)' -count=3   # ok
go test -tags integration ./internal/server -run RIExchange -count=1                                         # ok

New tests in store_postgres_ri_exchange_resolve_integration_test.go:

• The issue's scenario. A $1000 hold back-dated 7h blocks a new AUTO reservation (ErrRIExchangeDailyCapExceeded) and still counts two days later. Resolving it failed makes the same reservation succeed with a $1000 ceiling.
• completed keeps the $700 counted today and not tomorrow, and stores the operator's exchange id.
• A row changed 5h59m ago is refused for both outcomes and stays processing.
• A row with a recorded exchange_id cannot be released, can be completed, and rejects a mismatched exchange id.
• The cloud check is validated: other account, other region, checked before or at the last change, in the future, failed+exchange id, bad outcome, empty note. An unknown id gives not-found.
• Replay of an identical request, and conflict for a different one.
• Race with a late settlement, in both lock orders. A third transaction holds the row lock until both calls are parked (checked via pg_stat_activity). The final status is always completed.
• Two concurrent resolves (failed vs. completed): exactly one wins and the other gets 409.

Unit / mock / fixture:

go test ./internal/api ./internal/server ./internal/config ./internal/mocks -count=1   # ok (non-race)

• handler_ri_exchange_stale_test.go: a stranded row produces exactly one ERROR line, with the fields and resolve path, while automation is disabled. A failing stale query is itself an ERROR. No stale rows means no ERROR.
• handler_ri_exchange_resolve_test.go covers:
  • what reaches the store: outcome, note, cloud check, actor, and the 6h threshold;
  • authorization: allowed with a region constraint that matches; 403 for view-only, for the admin API key, and for a constraint on another region;
  • 404 when the account is out of scope;
  • 17 malformed bodies, all 400 without reaching the store;
  • mapping of store errors to 400/404/409/500;
  • that the route is registered as AuthUser POST.

Fail-before / mutation probes. Each mutation below was applied alone, then restored, and a test fails under each:

Mutation	Failing test
handleRIExchangeReshape without the alarm call (= origin/main behavior)	TestHandleRIExchangeReshape_AlarmsOnStrandedProcessingExchange
Alarm moved after the enabled check	same
Staleness check removed	…_RefusesRowThatMayBeInFlight
Exchange-id guard on release removed	…_RefusesToReleaseRowWithExchangeID
Status check + UPDATE guard removed	…_ReplayAndConflict
checked_at after last change removed / account check removed	…_ValidatesCloudCheck
completed_at not stamped	…_CompletedCountsTodayOnly
Handler constraint check removed	TestResolveRIExchange_Authorization
acknowledge_unknown_outcome requirement removed	TestResolveRIExchange_BodyValidation

The late-settlement race is guarded four times over: the status check, the UPDATE guard, staleness (the settlement bumps updated_at), and the exchange-id guard. The race test fails only when all four are removed; with any one left in place it still passes.

On origin/main the new tests do not compile, because ResolveStaleRIExchange, RIExchangeResolveStaleAfter, alarmStaleRIExchanges and resolveRIExchange do not exist. The existing TestPostgresStoreDB_ReserveRIExchange_PriorDayInFlightHoldReducesHeadroom already shows the hold persisting across days.

Static checks.

• go build ./... and go vet (with and without -tags integration) pass on the changed packages.
• golangci-lint v2.10.1 (--build-tags integration and without) reports 0 issues.
• gocyclo -over 10 -ignore "_test\.go" internal/ cmd/ is silent.
• scripts/gosec-hook.sh on the changed files passes.
• git diff --check is clean.
• git-secrets: the changed files scan clean in an isolated repo (repo patterns plus --register-aws), and scripts/test-git-secrets-allowlist.sh gives 42 PASS / 0 FAIL.

Real-cloud gap. This was not exercised against a real AWS account or a deployed Lambda. I also did not verify that the CloudWatch errors metric filter ([time, request_id, level = ERROR*, ...]) matches the logging.Errorf line format (YYYY/MM/DD HH:MM:SS [ERROR] …). That applies to every existing ERROR log, not just this one, and is worth a check by someone with a deployed stack.

Deliberately out of scope

• Automatic provider lookup. The issue's proposed fix includes describing the source RIs at AWS and settling automatically. I left that out so nothing ever releases headroom on inferred evidence: a source RI that is still active, or one that is retired (it may have expired or been modified), is not proof either way. Automatic completion on strong evidence could be a follow-up.
• fix(ri-exchange): an unknown accept outcome is marked failed and retryable #1050. The manual-approval path still calls failExchange when Execute returns an unknown-outcome error, which releases a hold for an exchange that may have happened. That is tracked in fix(ri-exchange): an unknown accept outcome is marked failed and retryable #1050 and I did not change it here.
• Late FailRIExchange. CompleteRIExchangeWithPayment/FailRIExchange still update unconditionally. A writer still alive more than 6h later could flip a resolved row. I judged that out of reach, but tightening those writers to WHERE status = 'processing' is a possible follow-up.
• Ladder adapter and the schedule toggle. The ladder adapter does not call the alarm itself. Its rows are covered by the reshape schedule's origin-agnostic query, but not on deployments where the RI exchange schedule is disabled.
• 4-eyes. The resolve endpoint is not gated by 4-eyes mode: it corrects the ledger, it does not execute.
• No frontend UI (frontend untouched) and no runbook. The alarm line itself says how to resolve.

A maintainer, please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, urgency/this-quarter, impact/few, effort/m, type/bug, triaged.

Closes #692

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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

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

[cristim]

VERDICT: APPROVE 89c1363

Reviewed head 89c1363 (1 commit) against origin/main b6a0649. The base is 40 commits stale; git merge-tree against origin/main is clean, and all tests below ran on the merged tree.

Findings (none blocking):

• process nit: the single commit changes 1127 lines, over the 400-line cap.
• nit internal/config/ri_exchange_reservation_integration_gate_test.go:15-19 (on main) - the new store_postgres_ri_exchange_resolve_integration_test.go is not in riExchangeReservationIntegrationFiles, so the test(ri-exchange): daily-cap reservation Postgres tests are env-gated and skip in CI #724 guard against env-gated skips does not cover it. It uses newRIExchangeIntegrationEnv, the container helper, so it does run today.
• low (acknowledged in the PR) internal/config/store_postgres_ri_exchange_resolve.go:195-208 - the operator write is guarded, but CompleteRIExchangeWithPayment/FailRIExchange are not. A writer still alive more than 6h after its last change could overwrite an operator "completed" with "failed" and release the hold. That is out of reach with the 4m30s and 15m budgets, but a WHERE status = 'processing' on those writers is a worthwhile follow-up (see also fix(ri-exchange): an unknown accept outcome is marked failed and retryable #1050).
• design note: the resolve endpoint is not gated by 4-eyes, so one operator holding execute:ri-exchange can release a hold. It moves no money: with fix(ri-exchange): run unattended exchanges as manual while 4-eyes is on #1108, every later exchange under 4-eyes still needs approval. The PR states this choice.

Attacked:

• Stale threshold: resolving needs 6h unchanged, measured on the DB clock (NOW() under FOR UPDATE). The alarm fires at 1h, and its text says resolving is allowed once the row is 6h unchanged.
• Who can resolve: execute:ri-exchange plus Constraints (account, aws, ec2, checked_region) and the deployment-account scope (404 when out of scope). The admin API key is refused. A user API key is allowed and stamped with its owner's UUID.
• Audit trail: the decision is written to error/transitioned_by/transitioned_at on the row, plus a WARN log when it is not a replay.
• Double-execute and money state: no provider call anywhere on the path. The row lock, the status check, the AND status='processing' UPDATE guard, the staleness check and the exchange-id guard all apply. A replay requires the identical note and writes nothing.
• Unresolvable "unknown" account rows: refuted, because migration 000009 CHECKs account_id against ^\d{12}$.
• Alarm wiring: logging.Errorf prints [ERROR] (pkg logging/logger.go:211), and main's errors metric filter is "[ERROR]" (monitoring/aws/main.tf:341, fixed by chore(iac): no alert reaches anyone; instantiate monitoring and wire failure notifications #714), so the PR body's caveat no longer applies. No terraform is touched. The alarm runs before the enabled check, and both schedules exist only when enable_ri_exchange_schedule is set.
• Mutation probes (scratch copy of the merged tree, each reverted). Each of these makes a test fail: alarm call removed, ack requirement removed, admin key allowed, constraint check removed, scope check removed, view instead of execute permission, staleness check removed, exchange-id release guard removed, completed_at not stamped, account check removed, and status check plus UPDATE guard removed.

Local results (merged tree 8afef041, macOS, Docker Postgres container, AWS creds disabled): go build ./... OK; go vet with and without -tags integration on api/server/config OK; unit -run 'Resolve|RIExchange|Stale|Route' across api/server/config: 663 passed; go test -tags integration -run 'PostgresStoreDB_(Resolve|Reserve|DailySpend)' ./internal/config/: 16/16 PASS; go test -tags integration -run RIExchange ./internal/server/: ok. CI on the PR head is all green, but it ran on the stale base.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 89c1363

Findings (none blocking):

• [non-blocking, test gap] internal/api/handler_ri_exchange_resolve_test.go:153 - The "bad outcome" case in TestResolveRIExchange_BodyValidation has no acknowledge_unknown_outcome.
  • The ack check (handler_ri_exchange_resolve.go:185) rejects it first, so the outcome check (:163) is never exercised.
  • Replacing that case with case false: passes the whole suite.
  • Production is still safe, because validateRIExchangeResolve (store_postgres_ri_exchange_resolve.go:115) rejects any other outcome.
• The PR body's mutation-probe table holds up: I confirmed 9 of its probes and refuted none.

Against #692:

• The root cause is a stranded processing hold that stays counted forever (spend query at store_postgres.go:3551-3558).
• The PR adds an alarm and an operator release path, which is the issue's fallback.
• It leaves out automatic provider reconciliation and says why: there is no proof of the outcome. That is a defensible fail-safe choice, consistent with the issue's "counting an ambiguous row conservatively until then is intended".

Attacked:

• Routing and auth: /api/ri-exchange/resolve/ is not among the isPublicEndpoint prefixes (middleware.go:19-36), so it is AuthUser. No ExactPath collides with it, and production passes *PostgresStore (app.go:775).
• Staleness: the ri_exchange_updated_at trigger (migration 000009:29) bumps updated_at on Complete/Fail, so both the staleness guard and the "checked_at after last change" guard see late writers.
• Accounting:
  • processing to completed with completed_at=NOW() is net zero for today's spend and drops off tomorrow.
  • A release only lowers spend, so skipping the global advisory lock is safe.
• Merge: merge-tree --write-tree with origin/main b6a0649 is clean. Main's changes to handler_ri_exchange.go, interfaces.go and mocks do not conflict, and the duplicated pkg/logging import merges to one. No migration.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
  1. Staleness check removed: RefusesRowThatMayBeInFlight FAILS.
  2. The UPDATE's AND status='processing' removed: all pass. It is redundant with the FOR UPDATE status check, as the PR states.
  3. Future checked_at check removed: ValidatesCloudCheck FAILS.
  4. Region check removed: ValidatesCloudCheck FAILS.
  5. Replay note comparison removed: ReplayAndConflict FAILS.
  6. The recorded-exchange_id release guard removed: RefusesToReleaseRowWithExchangeID FAILS.
  7. Handler constraint check removed: TestResolveRIExchange_Authorization FAILS.
  8. Admin-key refusal removed: Authorization FAILS.
  9. Account-scope check removed: OutOfScopeAccount FAILS.
  10. Alarm call removed: TestHandleRIExchangeReshape_AlarmsOnStrandedProcessingExchange FAILS.
  11. Handler outcome check removed: survives. This is the test gap above.

Verified locally, with no cloud credentials and a throwaway Postgres 17:

• Head:
  • go build ./...: ok. go vet, plain and with -tags integration, on api, server, config and mocks: ok. gofmt -l on the 7 changed files: clean.
  • go test -count=1 on the 4 packages: ok.
  • Config integration (-run 'PostgresStoreDB_(Resolve|Reserve|DailySpend)'): 16 PASS.
  • Server integration (-run RIExchange): 17 PASS and 1 FAIL, in TestRIExchangeAutoWorkers_ConcreteStoreAndFixtureClient.
    • That test is in a file the PR does not touch, and it fails at DB connection setup in the base harness.
    • It passes on the merged tree.
• Merged tree: build, vet, gofmt and unit tests ok. Config integration: 16 PASS. Server integration: 18 PASS.
• Fail-before: the 3 new test files on base do not compile, since the new symbols are undefined.
• No migrations.

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