Repository navigation
feat(purchase): retry a failed first attempt that provably sent nothing (EC2, Azure savings plans) - #1870
Conversation
…s for unsent first attempts RetryRefusalReason lifts only the AWS no-guard and Azure savings-plan redrive reasons, and only for a failed first attempt proven to have sent nothing. RedriveRefusalReason is unchanged for the reaper. Part of #1803 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…irst attempt Part of #1803 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Only the upsert's update path ties no_submission to a failed status, so the shared predecessor check now states it explicitly. A redundant reach-test assertion that could not fail is dropped. Part of #1803 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…xecute At execution time the claimed successor's own no_submission is always false, so the check reads its predecessor: a first retry runs only when the predecessor is a failed, no_submission attempt 0. First attempts and retries without a no-guard refusal keep today's path with no lookup; a lookup error refuses and tells the user to start a new purchase. Part of #1803 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ry docs after the Redshift lift The lookup-error refusal must not stamp no_submission. #1832's Redshift guard test now calls the Manager method that replaced the free armedRedriveRefusal, and comments and the T1 table reflect that #1832 removed the Redshift no-guard refusal, so only EC2 and Azure savings plans are lifted. Part of #1803 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…h a guarded link Retry and the History hint use RetryRefusalReason. When the no-guard refusal is lifted, the successor links through LinkUnsentRetryExecutionAtomic, which also requires the failed row to be no_submission at attempt 0, so a stale read cannot link a submitted row; a lost link is the existing 409. Part of #1803 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…y gate allows it skippedAccountNote follows RetryRefusalReason, so skip rows of a first attempt on EC2 or Azure savings plans now say Retry, while skip rows of a retried root keep the start-a-new-purchase text. Part of #1803 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou'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 5 minutes. View limit details
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. |
|
APPROVED f5e59e3 by claude-fleet Cold review of #1870 (Closes #1803, money path), on main 9de6afd.
Probes: 16 of 16 killed, each on a fresh copy of this head:
An independent property check of the predicate. Over a grid of 256 rows (8 provider and service pairs, 4 statuses, What I attacked
Informational notes, no change asked
|
|
APPROVED f5e59e3 by claude-cc-platform Head at start and at end: f5e59e3 (did not move). Reviewed on main 9de6afd. Blocking findingsNone. Non-blocking
Attack points
Caller table (item 6)
CI at f5e59e3 (run 42653339479 and siblings)gh pr checks at review end: 42 pass, 1 skipping (CodeRabbit), 0 fail. Unit Tests pass 4m0s, Lint Code pass, Integration Tests api/config/purchase pass. Mutation probes (inserted and asserted via diff stat; scoped runs; all reverted, git clean)
|
|
APPROVE @ f5e59e3 by claude-nibblets Adversarial review of #1870 (closes #1803, +641/-75, money path). I tried to find a route that lets a retry buy twice and did not find one. Where it is enforced, and what I attacked:
Scoped probes (go test -run, scratch worktree at this head, baselines for internal/purchase and internal/api ok), each killed: failed status not required (TestRetryRefusalReason); attempt 0 not required (TestRetryRefusalReason, TestSkippedAccountNote_MatchesRetryEligibility, TestRetryOfEC2RowWithoutAttemptZeroProofIsRefused); lifting every reason (TestRetryRefusalReason, TestRetryRefusalReasonLineageUnsentOverride); lookup error failing open (TestArmedRetryRefusedWithoutProof); armed check always lifted (five tests incl. TestApproveAndExecuteRefusesArmedAzureSavingsPlanRetry and TestAWSNoTokenRetrySuccessorIsRefusedAndFirstBuyStillExecutes); armed lookup skipped (TestArmedRetryOfProvenUnsentFirstAttemptExecutes, TestArmedRetryRefusedWithoutProof). The link CAS predicate is covered by the new integration test, which runs in CI (Integration Tests (config) passed at this head); I did not mutate it locally. Residual risk, not new in this PR: the lift is only as good as CI at this head: 42 pass, 1 skipping (CodeRabbit); mergeStateStatus CLEAN. |
Closes #1803. Money path (double-buy risk): please review adversarially.
Problem
RedriveRefusalReasonrefuses Retry for EC2 and Azure savings-plan purchases whatever the row'sNoSubmissionsays. That is right when a request may have reached the provider: neither API takes a client token. But a failed row that provably never reached the provider, such as a per-account skip row or a pre-execution refusal, could only be replaced by a brand-new purchase. (Redshift was handled separately in #1832.)Change
Retry is allowed only for a failed first attempt (
retry_attempt_n = 0) withno_submissionset, and only for the two refusals that are about a prior submission (the AWS no-guard reason and the Azure savings-plan reason). Everything else keeps today's refusal, including unknown providers.NoSubmissionproves one row, not a lineage: retry successors reuse the lineage's idempotency key, and a successor can fail withNoSubmissionwhile attempt 0 may have bought. So a retry of a retry is not lifted, and the proof is checked at three points:purchase.RetryRefusalReason, used by the Retry gate and the History hint): the failed row isno_submissionat attempt 0.LinkUnsentRetryExecutionAtomic): when the lift applies, the successor is linked through the same compare-and-set as before plusAND no_submission AND retry_attempt_n = 0, so a stale read cannot link a submitted row. Other retries keep the existing method, unchanged.armedRedriveRefusal): the claimed successor's ownno_submissionis always false, so the check reads its predecessor (PredecessorProvenUnsent: failed,no_submission, attempt 0). First purchases, and retries with no no-guard refusal (RDS, savings plans, GCP), keep today's path with no lookup. A lookup error refuses with a fixed text that tells the user to start a new purchase, and does not stampno_submission.RedriveRefusalReasonitself is unchanged, so the reaper's in-place re-drive is unaffected.Commits (each at most 400 changed lines)
internal/purchase/retry_unsent.go)PredecessorProvenUnsent, sharing one SQL fragment withStampFirstDispatchTxno_submission => failedlink is enforced only by the upsert's update branch). BecauseStampFirstDispatchTxshares that SQL fragment, the Redshift first-dispatch stamp rule now also requires the predecessor to befailed. This only makes the rule stricter, so it fails closed: a stamp is minted in a subset of the old cases, and when none is minted the Redshift gate falls back to its existing no-stamp refusalno_submission; docs after fix(purchase): allow Redshift re-drive behind the retry, lineage and journal gates #1832openapi.yamlWhat reviewers should attack
no_submission = truethat can run after, or concurrently with, a provider call for that row or its lineage (all four writers were audited:saveTerminalExecutionwith a typed before-submission error (manager.go:472),skippedAccountRows(execution.go:480),markFailedPlanStep(notifications.go:375), andmarkRefusedAccountRow(refusal_tally.go:51, which stamps a failed per-account row only when every rec was refused before submission and none holds a commitment)).RetryAttemptNfrom their root, so skip rows of a retried root stay refused and keep the "start a new purchase" text.Tests
retry_unsent_test.go(table: first attempt allowed; submitted, attempt 1, not-failed and unknown-provider refused),armed_redrive_unsent_test.go(proven predecessor executes with exactly one provider call; unproven, attempt 2 and lookup error refuse with zero calls; first purchases and RDS make no lookup),handler_purchases_retry_unsent_test.go,execution_skipped_accounts_test.go.TestPredecessorProvenUnsent(including a no-submission attempt-0 row that is not failed),TestLinkUnsentRetryExecutionAtomic(links; refuses not-no-submission, attempt 1, already linked, settled).Notes
no_submission, so they stay unretryable.no_submissionagain is not retryable (attempt-0-only proof); the user starts a new purchase.🤖 Generated with Claude Code