[IAmJSD]

Root cause

notifyExecutionFailure (the purchase_execution_failed log marker plus the purchase-failed email) was only called from executeAndFinalize and from each per-account fan-out child in executeForAccount. Two gaps followed.

1. Fan-out root (the issue as filed, reproduced). executeAndFinalize skipped the root of a multi-account fan-out (isFanOutOutcome) because it assumed every child had already notified. Two kinds of child never do. FanOutWithConcurrency records ctx.Err() for an account it skips at the semaphore once the context is done, and it recovers a panicking child (internal/execution/fanout.go). In both cases the child never stamps or saves its row, so its deferred notify sees a non-terminal status and does nothing. The root then ended failed or partially_completed with no marker and no email. The audit-loss child path was already covered: the child stamps its status before the save, so its deferred notify still fires. A test now pins that.
2. Other terminal-failure writers (maintainer scope from the issue comments). These writers never called notifyExecutionFailure at all:
   • failClaimedBeforeSubmission: post-claim cap re-check and ClaimedRowCheck refusals.
   • safeFail: stranded approved rows that RecoverStrandedApprovals fails.
   • the reaper (reapOne).
   • failClaimedRedrive (from fix(purchase): re-drive the claimed row, not the pre-claim snapshot #1095): a stranded row the recovery sweep claimed but whose claimed state no longer qualifies for a re-drive.
   • the plan-step failure paths: failExistingPlanStep, and the new failed row that failStep records inside the notification tick's transaction.

Fix

• failure_notify.go: the marker logging is split into logExecutionFailure. A new notifyRootFailure handles the root row in executeAndFinalize. A fan-out root always logs the marker (so the alarm fires) and sends no email, as the issue proposed: children that did notify are not mailed twice, and the root has no per-rec Purchased flags it could list. Every other root works as before.
• Each writer listed above now notifies, and only after it has won the row, so racing workers cannot both alert:
  • failClaimedBeforeSubmission holds the running claim. It notifies even when the terminal save fails, the same way executeAndFinalize does.
  • safeFail notifies only after winning the approved -> failed CAS. A lost CAS stays quiet.
  • failClaimedRedrive holds the running claim and notifies even when the terminal save fails, like failClaimedBeforeSubmission.
  • failExistingPlanStep notifies only after its transaction commits. Since fix(purchase): fail a plan step's root row atomically with its reason and suppression release #1176 the pinned CAS, the reason stamp and the suppression release share one transaction, so a lost CAS or any error before commit leaves the row pending and sends nothing.
  • A new failed plan-step row is notified only after the claim transaction commits. The plan row lock means only one tick records it, and stepClaim.failedNew now carries that row instead of a bool.
  • The reaper notifies after its won CAS, unless the annotation save finds the executor already recovered the row (ErrExecutionNotInExpectedStatus). That purchase did not fail, and TestSingleClaim_ReaperInterleavingRecoversPurchasedRow covers this interleaving on real Postgres. The annotation step moved into annotateReapedRow, and the counters behave the same as before.

No schema or migration changes.

Closes #1195

Verification

All evidence is unit tests with mocks plus local Postgres integration tests (testcontainers). No real cloud, SMTP or SES calls were made. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid.

New tests:

• failure_notify_writers_test.go tests the fan-out root:
  • TestLogExecutionFailure_LogsMarkerOnlyForFailedOrPartialRows pins the marker line itself. It catches the review's M11 mutation: deleting the logging.Errorf now fails this test and 3 others.
  • TestNotifyRootFailure_FanOutRootLogsMarkerWithoutEmail covers the skipped-child and panicked-child aggregates.
  • TestFanOutRootAlertsWhenPanickedChildNeverNotifies runs end to end through the SQS executor. The child's credential load panics, and exactly one root marker and no email are expected.
  • TestFanOutAuditLossChildStillNotifiesAndRootAddsMarker covers a child whose save fails: the child sends the marker and an email, and the root sends a marker and no email.
• failure_notify_paths_test.go has TestTerminalFailureWritersAlertOncePerWonRow. Each writer is run with a won CAS, a won CAS whose follow-up save fails, and a lost CAS. Each case asserts exactly one marker and one email, or none. For failExistingPlanStep a failed reason save or suppression release rolls the transaction back, so those rows expect no alert.
• Real-Postgres race tests (-tags integration):
  • TestFailExistingPlanStep_ConcurrentTicksReportOnce: 4 goroutines fail the same pre-created step row, and exactly one email is sent.
  • TestGetOrCreateExecution_ConcurrentTicksMintOneRow now also asserts one failure email across 4 racing ticks.
  • TestAttachIntegration_NoMatchAndCap asserts one email for the cap-failed step.
• TestRecoverStrandedApprovals_ClaimedRowTurnedUnsafeIsFailedNotBought (from fix(purchase): re-drive the claimed row, not the pre-claim snapshot #1095, real Postgres) now also asserts exactly one purchase-failed email for the claimed row that failClaimedRedrive fails.
• Updated existing tests whose rows end failed so they expect the one email: TestClaimPin_PostClaimPlanReadErrorFailsClosed, TestManager_RecoverStrandedApprovals_FailsStrandedRow, and TestNotificationTickOn... (renamed ...SendsNoApprovalEmail, since it now sends the failure email but still no approval email).

Fail-before/pass-after: I reverted only the production changes (the fan-out root back to marker-free, and the new notify calls removed) and kept the new tests. Every new and updated test above failed (16 failing tests/subtests), and all pass with the fix.

Commands run (all green):

go build ./...
go vet ./internal/purchase/ && go vet -tags integration ./internal/purchase/
go test ./internal/purchase/ ./internal/mocks/ ./internal/server/... ./internal/api/... ./internal/execution/ ./internal/email/ ./terraform/modules/monitoring/ -count=1
go test -short -race ./internal/purchase/ -count=1
go test -tags integration ./internal/purchase/ -count=1          # full package, real Postgres
go test -tags integration ./internal/api/ -count=1 -run 'Integration|Race|Claim|RunNow|Ladder|Combined|Retry'
golangci-lint v2.10.1 run --allow-parallel-runners [--build-tags integration] ./internal/purchase/...   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/purchase/          # silent
scripts/gofmt-hook.sh, scripts/gosec-hook.sh (changed files), git diff --check
git-secrets --scan on the changed files in a throwaway repo (a planted key was flagged; the real files were clean)
scripts/test-git-secrets-allowlist.sh                            # 137 PASS

Coverage gaps: the alarm wiring (terraform/modules/monitoring/*) is unchanged, the token and format are unchanged, and TestMonitoringLogMarkersMatchGo still passes. No deployed alarm or real mail delivery was exercised.

Behavior notes for review

• A fan-out with N failed children now logs N+1 markers: one per child that notified, plus the root. The alarms fire on any occurrence, so this only raises the count, not the number of alarms.
• Rare double alert: if the reaper fails a running row while a live executor later fails it as well, both the reaper and executeAndFinalize (on its audit-loss path) alert for that row. The 10-minute reap threshold sits well above the 270s executor budget, so this needs an executor that outlived its Lambda.
• The purchase-failed email template ("Some purchases could not be completed. Please review and retry manually.") is generic. For reaped and safeFail rows the row's Error text explains that the commitment may exist, but the email does not. The template is unchanged here.

Out of scope / follow-ups

• The "Smaller items" in the issue body are not addressed here, and a maintainer may want separate issues for them so Closes does not drop them:
  • The SMTP dial does not honor the 30s notify context. SMTPSender.dispatchSMTP and sendMailTLS never receive the context (smtp.Dial and smtp.SendMail), which affects every SMTP send, not just this path.
  • The AWS budget has no headroom. purchaseExecutionTimeout (4m) + terminalSaveTimeout (30s) + the detached failureNotifyTimeout (30s) after the save = 300s = lambda_timeout. Fixing it means either raising the Terraform/CFN floor and the tfvars (a deploy-affecting change) or shrinking a budget, so it needs a maintainer decision. This PR adds no notify inside the purchase budget: the new call sites run before the purchase, in sweeps, or in the notification tick.
• api.(*Handler).finalizePurchaseStatus marks a row failed when the approval email cannot be sent. It is left unchanged: that failure is returned synchronously to the user who submitted, and it is a mail failure, so sending another email would not help.
• Overlap with other PRs:
  • fix(purchase): fail a plan step's root row atomically with its reason and suppression release #1176 (merged) moved failExistingPlanStep's CAS into the transaction; this branch now notifies after that transaction commits.
  • fix(purchase): re-drive the claimed row, not the pre-claim snapshot #1095 (merged) added failClaimedRedrive; this branch now notifies from it.
  • fix(purchase): guard the remaining post-CAS full-row saves on status #1111 also touches manager.go and notifications.go.

Frontend follow-up for the UI owner

None.

Labels: please mirror the issue's triage labels on this PR: priority/p2, severity/medium, impact/internal, triaged, urgency/this-quarter, effort/s, type/bug.

Known unrelated CI failure: govulncheck (GO-2026-6809, crypto/x509 in Go 1.26.9) may fail "Security Scanning" / "Build Docker Image" until #1236 lands.

🤖 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.}

[IAmJSD]

Merged origin/main (a14dc8f) into the branch to resolve the conflict in internal/purchase/notifications.go. There were no review findings on the previous head (cb66e84) to address.

Conflict: internal/purchase/notifications.go (failExistingPlanStep, vs #1176)

• fix(purchase): fail a plan step's root row atomically with its reason and suppression release #1176 moved the pinned CAS, the reason stamp and the suppression release into one transaction (failPlanStepTx). I kept that and moved this PR's alert to after the commit (notifications.go:324-326).
• Before the merge, a failed reason write still alerted, because the CAS had already committed. That reason no longer holds: a lost CAS or any error before commit rolls back and leaves the row pending for the next tick, so nothing alerts.
• failure_notify_paths_test.go:120-135 changes to match. A rolled-back reason save, and a rolled-back suppression release (new row), now expect no marker and no email.

Overlap with #1095 (failClaimedRedrive, auto-merged)

• As this PR's body said, the alert belongs in whichever PR merged second. stranded_redrive.go:48 adds defer m.notifyExecutionFailure(ctx, exec).
• The worker holding the running claim alerts once, even when its terminal save fails, the same way failClaimedBeforeSubmission does.
• Tests:
  • Two new rows in failure_notify_paths_test.go:63-77.
  • fix(purchase): re-drive the claimed row, not the pre-claim snapshot #1095's TestRecoverStrandedApprovals_ClaimedRowTurnedUnsafeIsFailedNotBought now asserts exactly one purchase-failed email (redrive_claimed_row_integration_test.go:140-160). Before that change it panicked on the unexpected SendPurchaseFailedNotification.

PR body updated: the writer list, the plan-step and redrive bullets, the test list and the overlap notes. Closes #1195 is unchanged.

New head: 0bd8bf0

Verification (merged tree, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null):

• go build ./...
• go vet on ./internal/purchase/, with and without -tags integration
• go test ./internal/purchase/ ./internal/mocks/ -count=1
• go test -short -race ./internal/purchase/
• go test -tags integration ./internal/purchase/: all 404 tests pass. On a loaded shared runner the package ran in name-chunks, each with -timeout 20m. Three tests hit the testcontainers "container status removing" / "matched 0 times" startup error and passed when rerun alone.
• Mutation probes:
  • Removing the failClaimedRedrive notify fails both new unit rows and the fix(purchase): re-drive the claimed row, not the pre-claim snapshot #1095 integration assertion.
  • Moving the plan-step notify back inside the transaction fails both rolled-back rows.
• golangci-lint v2.10.1, with and without --build-tags integration: 0 issues.
• Clean: gocyclo -over 10 (changed non-test files), gofmt, scripts/gosec-hook.sh, git diff --check.
• git secrets --scan on the changed files in a throwaway repo: clean, and a planted key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS.

The govulncheck jobs may still fail on GO-2026-6809 (Go 1.26.9) until #1236 lands. That is unrelated to this PR.