[IAmJSD]

Why

#1245 made every writer that ends an execution row failed log the purchase_execution_failed marker and send the purchase-failed email. That includes the recovery sweeps: the stuck-row reaper (reapOne), the stranded-approval sweep (safeFail), and failClaimedRedrive, which fails a claimed stranded row the same way safeFail does. Those rows are failed because the purchase run was interrupted, so the commitment may already exist. The generic email ("Some purchases could not be completed. Please review and retry manually.") lists every selected recommendation as a failed purchase and tells the operator to retry, which could buy the same commitment twice.

Closes #1415

What

• internal/email: NotificationData.OutcomeUnknown switches RenderPurchaseFailedEmail to a new purchaseOutcomeUnknownTemplate, and both senders to the subject "CUDly - Purchase Outcome Unknown - Verify in the Cloud Console" (purchaseFailedSubject). The body:
  • names the execution (and plan, if set),
  • says the run was interrupted, the provider may already have created the commitments, and each one should be checked in the cloud provider's console before buying it again,
  • lists only that row's selected recommendations not recorded as purchased, under "Outcome unknown:", and
  • never contains "retry".
    With the flag unset, the email is byte-for-byte unchanged.
• internal/purchase: notifyOutcomeUnknown logs the same marker as notifyExecutionFailure, so the alarm and the once-per-won-row rule from fix(purchase): alert on failed fan-out roots and every terminal-failure writer #1245 stay as they are, and sends the flagged email. safeFail, failClaimedRedrive and the reaper now call it. Everything else fix(purchase): alert on failed fan-out roots and every terminal-failure writer #1245 did is unchanged:
  • the fan-out root still logs the marker and sends no email;
  • failClaimedBeforeSubmission, failExistingPlanStep, the new failed plan-step row and the executor paths keep the generic email;
  • the reaper still stays quiet when the executor recovered the row before the annotation;
  • a lost CAS still sends nothing.
• failClaimedRedrive is included although the issue names only the reaper and safeFail. Its doc says the outcome is the one safeFail gives (the interrupted original run may already have bought), so it gets the same email.

No schema, migration or Terraform change. The marker token is unchanged.

Verification

All evidence comes from unit tests with mocks, a local SMTP mock server, and local Postgres integration tests (testcontainers). No real cloud, SES or SMTP calls were made.

• TestTerminalFailureWritersAlertOncePerWonRow (internal/purchase/failure_notify_paths_test.go) now renders each email a writer sends with email.RenderPurchaseFailedEmail:
  • Reaped and swept rows (safeFail won CAS, with and without the reason save failing; claimed redrive row, with and without the terminal save failing; reaper won CAS, with and without the reason save failing): the body has no "retry" in any case, has no "Failed Purchases", names execution exec-w (plan plan-w), asks for the console check, and lists the row's recommendation under "Outcome unknown:".
  • Genuinely failed rows (plan step won CAS; claimed row refused before submission, with and without the terminal save failing): the body must equal the pre-change email exactly.
  • Fail-before/pass-after: on unfixed origin/main (7cde88e) with only this test file copied in, the 6 reaped/swept subtests fail and the 3 genuine-failure subtests pass. With the fix, all pass.
• internal/email/purchase_outcome_unknown_test.go: render content with and without a plan ID; the subject helper (including the 100-byte SNS subject limit); and the SMTP sender against the mock server, checking the subject and body for both a genuine failure and an outcome-unknown row.

Commands (all green):

go build ./... && go vet ./... && go vet -tags integration ./... && (cd tests/e2e && go vet -tags e2e ./...)
golangci-lint v2.10.1 run [--build-tags integration] ./internal/email/... ./internal/purchase/...   # 0 issues
gocyclo -over 10 <changed non-test files>                                                          # silent
go test -race -short -count=1 ./internal/email/ ./internal/purchase/
go test -race -tags integration -count=1 ./internal/purchase/                                      # real Postgres
secrets-scan on changed files: clean; planted key flagged
scripts/test-git-secrets-allowlist.sh                                                             # 137 PASS

Gaps: no real mail delivery was exercised. The SES Sender subject goes through the same purchaseFailedSubject helper, which has a unit test, but no SES send was made.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 11 billable files and costs up to $2.75.}

Or wait 57 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 51957bc
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 25fa47c3-73ea-4690-9254-e7d0074159ee

Commits

Reviewing files that changed from the base of the PR and between 51957bc and a302b8c.

Files selected for processing: 11

• internal/email/purchase_outcome_unknown.go
• internal/email/purchase_outcome_unknown_test.go
• internal/email/sender.go
• internal/email/smtp_sender.go
• internal/email/template_renderers.go
• internal/email/templates.go
• internal/purchase/failure_notify.go
• internal/purchase/failure_notify_paths_test.go
• internal/purchase/manager.go
• internal/purchase/reaper.go
• internal/purchase/stranded_redrive.go

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

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

[IAmJSD]

Merged origin/main (26004a6) to pick up #1111 and #1418. #1418 registers failedEmailRecorder in TestPostCASSave_ConcurrentStatusChangeSurvives/safeFail stamp after a late recovery and asserts at most one alert per row. safeFail still sends exactly one SendPurchaseFailedNotification per won CAS, now with OutcomeUnknown set, so that invariant holds. The merge was clean, with only an auto-merge in internal/purchase/manager.go.

New head: 94e64ecd (94e64ecdaad0857a5678bd20855fba673211b47f).

Checks on the merged tree:

• go build, go vet (plain, integration, e2e): clean.
• golangci-lint v2.10.1 on internal/{email,purchase}, with and without --build-tags integration: 0 issues.
• go test -race -short on ./internal/email/ and ./internal/purchase/: ok.
• go test -race -tags integration ./internal/purchase/: ok. This includes TestPostCASSave_ConcurrentStatusChangeSurvives (all 3 subtests) and TestRecoverStrandedApprovals_ClaimedRowTurnedUnsafeIsFailedNotBought. The first attempt timed out building the shared Postgres template on a heavily loaded machine. That is environmental and also happens on unmodified main. The rerun passed.
• Secrets scan: clean, and the planted key was flagged.

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 94e64ecdaad0857a5678bd20855fba673211b47f

Blocking:

• none

Non-blocking:

• internal/purchase/execution.go:300 (executeForAccount) still sends the generic "retry manually" email for every executor error. An ambiguous provider error, such as a timeout or connection reset after the purchase request was submitted, has the same "the commitment may already exist" property as a reaped row. That's outside Reaped and swept purchases send a 'retry manually' email although the commitment may already exist #1415's scope (the issue names only the sweeps), but it's worth a follow-up issue: classify ambiguous executor errors as outcome-unknown.
• internal/email/purchase_outcome_unknown.go:16: the reaper also fails stuck approved rows that may never have started. For those, "was interrupted ... may already have created them" is conservative but slightly inaccurate. I'd keep it, because erring towards "verify first" is the safe direction on a money path.

Attacked:

• Fail-before: I reverted only internal/purchase/{manager,reaper,stranded_redrive}.go to origin/main and kept the email package and notifyOutcomeUnknown. All 6 reaped/swept subtests of TestTerminalFailureWritersAlertOncePerWonRow fail. Restoring the files makes them pass. The genuine-failure subtests compare the rendered body byte-for-byte against the pre-change template, so the unflagged path is pinned.
• Money path: the once-per-won-row invariant is unchanged. Both helpers go through sendFailureNotice, which keeps the logExecutionFailure marker gate, and each sweep still notifies only after winning its CAS or claim. I listed every SendPurchaseFailedNotification implementation (SES/SNS Sender, SMTPSender, Nop, mock): both real senders route through RenderPurchaseFailedEmail and purchaseFailedSubject, so neither can send the old "retry" body for a flagged row. The new subject is 63 bytes, under the SNS 100-byte limit. buildPurchaseFailedData still excludes recommendations already recorded Purchased, so a partly recorded row lists only the unknowns.
• Remaining notifyExecutionFailure callers (execution.go, notifications.go x2, step_cap.go) are genuine refusals or executor failures, which matches the issue's "keep today's email" list.
• Ran go build ./..., go vet (plain and -tags integration) on internal/email and internal/purchase, go test -race -short -count=1 ./internal/email/ ./internal/purchase/ and go test -race -tags integration -count=1 ./internal/purchase/ ./internal/email/ (testcontainers Postgres): all ok.

Body-stale: none

_{Reviewer: rB9. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 94e64ecdaad0857a5678bd20855fba673211b47f

Blocking:

• none

Non-blocking:

• internal/email/purchase_outcome_unknown.go:25: when every selected rec on a reaped row is already flagged Purchased (executor saved the flags, then died before the status write), the email prints "Outcome unknown:" over an empty list. Same shape as the old template's empty "Failed Purchases:", so not a regression, but a one-line "{{else}}none recorded as unpurchased" would read better.
• The reaper also reaps stuck approved rows whose executor may never have started; the copy says the run "was interrupted". That's the conservative reading and fine for safety, just slightly imprecise.

Attacked:

• Checked every SendPurchaseFailedNotification implementation (SES Sender, SMTPSender, NopSender, mock). Both real senders route the subject through purchaseFailedSubject, and the one renderer branches on OutcomeUnknown. No HTML variant exists that could be missed. Subject is 63 bytes, under the SNS 100-byte limit.
• Checked every notifyExecutionFailure caller. Only the three sweep writers (safeFail, failClaimedRedrive, reapOne) moved to notifyOutcomeUnknown. Executor, step-cap, plan-step and fan-out root paths are unchanged. The marker log and the once-per-won-CAS gate stay in the shared sendFailureNotice, so the alarm from fix(purchase): alert on failed fan-out roots and every terminal-failure writer #1245 is untouched.
• Fail-before: reverting manager.go, reaper.go and stranded_redrive.go to origin/main makes exactly the 6 sweep subtests of TestTerminalFailureWritersAlertOncePerWonRow fail. Restoring the files makes them pass. The genuine-failure subtests pin the old body byte-for-byte.
• Ran: go build ./..., go vet (with and without -tags integration), golangci-lint v2.10.1 on email+purchase (both tag sets, 0 issues), gocyclo -over 10 (silent), go test -race -short on email+purchase, and go test -race -tags integration on purchase+email (real Postgres). All green.

Body-stale:

• none

_{Reviewer: rB12. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Merged origin/main (which now has #1417) and resolved the conflict in safeFail. New head: a302b8c.

Resolution (internal/purchase/manager.go, safeFail): I kept #1417's notify gate and pointed its deferred call at notifyOutcomeUnknown instead of notifyExecutionFailure. The result:

• Lost guarded stamp (ErrExecutionNotInExpectedStatus, the row was recovered after the flip): no email (fix(purchase): skip the failure email when the guarded failed-status save loses #1417).
• Won flip on a swept row, stamp landed or failed for another reason: the outcome-unknown email (Reaped and swept purchases send a 'retry manually' email although the commitment may already exist #1415).
• The reaper and failClaimedRedrive did not conflict. They keep notifyOutcomeUnknown. The reaper's existing quiet path for an executor-recovered row is unchanged.
• Genuine failures (plan step, refused claimed row, executor paths) keep the generic email.

New test commit a302b8c: adds a "safeFail won CAS, row recovered before the stamp" case to TestTerminalFailureWritersAlertOncePerWonRow, which expects no alert. It mirrors the reaper case that was already there. Fail-before/pass-after: with the gate line disabled, this case fails. With the gate, it passes.

Checks (local only, no cloud calls):

• go build ./... && go vet ./... && go vet -tags integration ./... && (cd tests/e2e && go vet -tags e2e ./...): OK
• golangci-lint v2.10.1 on ./internal/email/... ./internal/purchase/..., with and without --build-tags integration: 0 issues
• gocyclo -over 10 on the changed non-test files: no output
• go test -race -short -count=1 ./internal/email/ ./internal/purchase/: ok
• go test -race -tags integration -count=1 ./internal/purchase/ (real Postgres via testcontainers): ok. TestPostCASSave_ConcurrentStatusChangeSurvives passed, including both fix(purchase): skip the failure email when the guarded failed-status save loses #1417 subtests ("safeFail stamp after a late recovery", which expects 0 emails, and "safeFail flip without a recovery notifies once"). TestTerminalFailureWritersAlertOncePerWonRow passed.
• Secrets scan on the changed files: clean; planted key flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS.

🤖 Generated with Claude Code

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE a302b8cb89d4a79df7e9e415eb960b85ccd07bd8

Blocking:

• none

Non-blocking:

• internal/purchase/reaper.go:279: a reaped row whose recs are all redrive-safe still gets the History error "...executor did not complete; safe to retry", while the new email (correctly) says verify in the console before buying again. A manual Retry mints a new row/ExecutionID, so the provider idempotency token would not dedupe it; the "safe to retry" wording predates this PR, but the two messages now disagree. Worth a follow-up issue rather than a change here.
• internal/purchase/reaper.go:287: if the reaper wins the CAS on a fan-out root row, it now sends an outcome-unknown email listing every selected rec of the root (roots carry no per-rec Purchased flags), and each reaped child row sends its own. That overlap comes from fix(purchase): alert on failed fan-out roots and every terminal-failure writer #1245, not this PR, and the new wording ("verify before buying") makes it harmless, but notifyRootFailure's "root sends no email" rule does not hold on the reaper path.
• The PR body does not mention the new subtest "safeFail won CAS, row recovered before the stamp" (fix(purchase): skip the failure email when the guarded failed-status save loses #1417) in failure_notify_paths_test.go. It is a correct addition, just undocumented.

Attacked:

• Checked that every writer that ends a row failed after an interrupted run is switched: grep found exactly safeFail, failClaimedRedrive and reapOne. The genuine-failure writers keep the generic email: failClaimedBeforeSubmission, step_cap, the notifications.go plan-step paths, executeForAccount and notifyRootFailure. All three senders are covered: SES and SNS go through purchaseFailedSubject, the subject fits the 100-byte SNS limit, the SMTP sender has a test, and the Nop sender is unaffected. The outcome-unknown template uses text/template and contains no "retry". With the flag unset, the old template renders byte-for-byte as before (the test asserts this with exact equality).
• Lost-CAS, recovered-before-annotation and recovered-before-stamp paths still send nothing. The marker/alarm still comes from the shared logExecutionFailure path.
• Fail-before: I reverted only the three call sites (manager.go, reaper.go, stranded_redrive.go) back to notifyExecutionFailure. TestTerminalFailureWritersAlertOncePerWonRow then fails in exactly the 6 reaped/swept subtests, and the genuine-failure subtests still pass. I restored the files afterwards.
• Ran: go build ./...; go vet with and without -tags integration on internal/email and internal/purchase; go test -race -short -count=1 on both (ok); go test -race -tags integration -count=1 ./internal/purchase/ (ok, 132s); golangci-lint v2.10.1 with and without --build-tags integration (0 issues); secrets-scan (clean; planted key flagged).

Body-stale:

• none

_{Reviewer: rB14. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE a302b8cb89d4a79df7e9e415eb960b85ccd07bd8

Blocking:

• none

Non-blocking:

• internal/email/purchase_outcome_unknown.go:16: the body always says the run "was interrupted before it recorded a result". The reaper also fails stuck approved rows that no executor ever claimed, and for those nothing was bought. The cautious wording is safe there (verify before buying again), just not exact. Wording like "may have been interrupted" would cover both cases.
• internal/email/purchase_outcome_unknown.go:25: if every selected recommendation is already flagged Purchased (or none are selected), the email prints an empty "Outcome unknown:" list. That is harmless but confusing. An {{else}} line such as "(none recorded)" would help.
• Pre-existing from fix(purchase): alert on failed fan-out roots and every terminal-failure writer #1245 and not changed here: if the reaper fails a fan-out root row, the root's email lists every recommendation across accounts, and each reaped child sends its own email too. The new template doesn't make that worse.

Attacked:

• Call sites: grepped every notifyExecutionFailure / notifyOutcomeUnknown / notifyRootFailure caller. Only safeFail (manager.go:719), failClaimedRedrive (stranded_redrive.go:48) and reapOne (reaper.go:287) moved to the outcome-unknown variant. failClaimedBeforeSubmission (step_cap.go), the plan-step paths (notifications.go) and executeForAccount keep the generic email, which matches the issue's "rows that really failed keep today's email". Marker logging and the won-CAS gating are unchanged because both variants share sendFailureNotice.
• Both real senders (SES Sender and SMTPSender) go through purchaseFailedSubject. NopSender and the mocks don't need it. The subject is 63 bytes, under the SNS limit.
• Fail-before: in the scratch checkout I reverted only the three call sites back to notifyExecutionFailure. TestTerminalFailureWritersAlertOncePerWonRow then failed exactly the 6 reaped/swept subtests, and the genuine-failure subtests still passed. I restored the files afterwards.
• Ran: go build ./..., go vet (plain and -tags integration) on internal/email and internal/purchase, go test -race -short -count=1 on both, go test -race -tags integration -count=1 on both (real Postgres, all ok), and golangci-lint on both (0 issues).

Body-stale:

• none

_{Reviewer: rB15. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at a302b8cb89d4a79df7e9e415eb960b85ccd07bd8 merged onto main (5f717aa4), together with #1260, #1261, #1263, #1264, #1269, #1271, #1273, #1308, #1416 in that order (combined tree 62127c84, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on a302b8cb (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly a302b8cb89d4a79df7e9e415eb960b85ccd07bd8.