Skip to content

feat(exchange): flag an exchange whose accept outcome is unknown - #497

Merged
cristim merged 1 commit into
mainfrom
feat/443-exchange-outcome-unknown
Oct 17, 2026
Merged

cristim merged 1 commit into
mainfrom
feat/443-exchange-outcome-unknown

Conversation

@cristim

@cristim cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member

Summary

Part 1 of 2 for #443. When an RI exchange accept returns an ambiguous result (common.ErrOutcomeUnknown, from the single-shot accept added in #299), the exchange may already have executed. RunAutoExchange halted the run and left the reservation record processing, but the outcome reached consumers indistinguishable from a definite failure.

pkg/exchange/auto.go:

  • New ExchangeOutcome.OutcomeUnknown bool, documented: the accept result was ambiguous, the exchange may have executed, reconcile before any retry, the outcome stays in AutoExchangeResult.Failed, and it halts the run.
  • Set in the existing ErrOutcomeUnknown branch. No other behavior changes.

Why a flag instead of a separate Unknown bucket: consumers that only read Failed (the platform's scheduled email, the ladder summary) keep seeing the outcome, including while their pkg pin lags. A new bucket would make it disappear for them.

Additive, so it compiles under both go.work and the GOWORK=off standalone provider build. ExchangeOutcome has no JSON tags and is not persisted; the scheduled task's result gains one key.

Out of scope here:

Refs #443

Tests

TestRunAutoExchange_OutcomeUnknownFlag drives RunAutoExchange:

  • Execute returning fmt.Errorf("x: %w", common.ErrOutcomeUnknown): flag true, one outcome in Failed, no fail-ledger call, Execute called once.
  • a plain error: flag false, one outcome in Failed, one fail-ledger call.

The existing TestRunAutoExchange_UnknownAcceptLeavesRecordProcessingAndHalts keeps covering the processing record and the halt.

Mutation probe (scratch copy): deleting the OutcomeUnknown = true assignment fails the wrapped-error case. Reproduced independently by the reviewer.

Verification

make build, make vet, (cd pkg && GOWORK=off go test -race -short ./exchange/...), (cd providers/aws && GOWORK=off go build ./...), pinned golangci-lint v2.10.1 on pkg (0 issues), gofmt, and the commit/pre-push hooks pass. Unit-level evidence only.

Local review: the plan had two adversarial passes by a non-author reviewer (which found the platform manual-path gap, now platform#1050); the diff had two more by the same reviewer, no actionable findings.

🤖 Generated with Claude Code

@cristim cristim added priority/p2 Backlog-worthy severity/medium Moderate harm urgency/this-sprint Within the current sprint impact/few Limited audience effort/s Hours type/bug Defect labels Oct 17, 2026
@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 2 billable files and costs up to $0.50.

  • 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 24 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 72 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-go/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 2b5e3b49-2a5c-4527-ba1c-42fe756409eb
📥 Commits

Reviewing files that changed from the base of the PR and between 8a85908 and 790c2ba.

📒 Files selected for processing: 2
  • pkg/exchange/auto.go
  • pkg/exchange/outcome_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

Independent review verdict: CLEAN at b230869 (part 1 of 2 for #443)

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief, focused on the money path. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none.

The flag (pkg/exchange/auto.go):

  • Unknown outcomes only: OutcomeUnknown = true is set only at :581, inside if errors.Is(execErr, common.ErrOutcomeUnknown) (:580, which sees through wraps).
  • Halt behaviour unchanged: that branch still returns outcome, true, so the run halts. It does not call failLedgerRecord, so the record stays processing.
  • Definite errors: these fall through to failLedgerRecord (:584) with the flag false.
  • Failed list: the outcome still lands in result.Failed (:285-286). Manual mode (:271-277) never sets the flag.
  • Shape: an additive bool on a struct with no JSON tags.

Consumers that still treat an unknown outcome as a plain failure, which part 2 must fix:

  • providers/aws/ladder/reshape.go:180-185 renders every Failed entry as "failed: …".
  • reshape.go:192-200 returns "N of M exchange attempt(s) failed".
  • Platform, on main: internal/server/handler_ri_exchange.go:95 (scheduled auto-exchange) and internal/server/ladder_write.go:73 (ladder adapter → reshape). Both receive AutoExchangeResult and should surface or skip-retry OutcomeUnknown entries once the pin is bumped.

What was attacked and how (pkg, GOWORK=off):

  • go vet ./exchange/... is clean. go test -race -count=1 ./exchange/... passes 167.
  • The test asserts len(Failed) == 1 and failCalls (0 for unknown, 1 for plain), which covers both the halt and the record staying processing.
  • Fail-before: deleting :581 fails "wrapped unknown sets the flag" (outcome_test.go:140).
  • Mutation probe: setting the flag on any error fails "plain error leaves it false".

Gate: p2. Merge once CI is green, in its turn in the go merge order.

When the accept response is lost, ErrOutcomeUnknown means the exchange may
have executed. RunAutoExchange kept the ledger record processing and halted
the run, but the outcome in Failed carried only an error string, so a reader
could not tell it from an ordinary failure.

Add ExchangeOutcome.OutcomeUnknown, set where ErrOutcomeUnknown is handled.
The outcome stays in Failed, so existing readers of Failed still see it.
Behavior is otherwise unchanged.

The manual approve and interactive paths call ExecuteExchange from the
platform and cannot receive this flag from pkg; they are tracked in
LeanerCloud/cloud-commitments-platform#1050.

Refs #443

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim
cristim force-pushed the feat/443-exchange-outcome-unknown branch from b230869 to 790c2ba Compare October 17, 2026 01:47
@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

Pure rebase onto main after go#496 merged. git range-diff of b230869 against new head 790c2ba shows the commit identical (=), so the CLEAN verdict carries. (claude-cc-reviewer)

@cristim
cristim merged commit b5e755f into main Oct 17, 2026
15 checks passed
@cristim
cristim deleted the feat/443-exchange-outcome-unknown branch October 17, 2026 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/few Limited audience priority/p2 Backlog-worthy severity/medium Moderate harm type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant