[IAmJSD]

Stacked on #1237

This branch is stacked on #1237 (fix/issue-1232, head 17654ccf) and must merge after #1237. Until #1237 merges, this PR's diff also shows #1237's commit (17654ccf, the guarded SaveExecutionIfStatus write). The change for this issue is the single commit 91827b9a on top. Once #1237 is in main, the diff shrinks to that commit.

Root cause

finalizePurchaseStatus (internal/api/handler_purchases.go) runs after the approval email send on the submit path (executePurchase) and the retry path (retryPurchase). When the send fails, it marks the new execution failed. With #1237 that write is SaveExecutionIfStatus(..., []string{"pending"}). If the write failed for any reason other than a lost guard (connection reset, a deadline, a missing row), the function logged the error and returned "pending". Both callers put that into a 200 response: {"status":"pending","email_sent":false}.

The row really was left pending, with an empty error column and a valid approval token, while no approval email existed. It looked exactly like a healthy request waiting on its approver, and nothing in History or resolveOpsHint explained it until the expiry sweep. This is the same silent save-failure pattern COR-10 fixed in LeanerCloud/cloud-commitments-cli#1237.

Fix

• finalizePurchaseStatus now returns (string, error). On a save failure (the default branch) it returns a 500 client error built by the new emailFailureNotRecordedError:
  • The message starts with AUDIT LOSS:. It names the execution and says it may still show as pending with no approval email on the way, so cancel it from History.
  • The body also carries execution_id, email_sent: false and email_reason, so the UI or an API client can find the row and cancel it.
  • The store error is wrapped with %w (still reachable with errors.Is and in the server log with an AUDIT LOSS: prefix) but kept out of the response body.
• executePurchase and retryPurchase return the error instead of a response. On retry, the original row's link to the successor is unchanged, and the 500 names the successor.
• The email-sent path, fix(api/purchases): guard the email-failure write in finalizePurchaseStatus on pending #1237's guarded write and its ErrExecutionNotInExpectedStatus (lost guard) branch are unchanged.
• The submit-path tail (send, finalize, build the response) moved into sendApprovalAndFinalize, so executePurchase stays at gocyclo 10.

Why a 5xx and not failed or a new status: the row was not written, so answering failed would be just as wrong as pending. It is also possible that the write committed and only the acknowledgement was lost, so the server cannot say which state the row is in. A new status value such as failed_send_audit_loss would have to be added to the status set and handled by every client. A 500 is the option the issue and the 2026-07-28 review comment prefer ("the response should be a 5xx so the caller retries or alerts rather than recording a phantom pending"), and it matches how cli#1237 handled the same case (it returns the AUDIT LOSS error). No execution_status values change and there is no migration.

Closes #54

Verification

Run on the Linux VM from the repo root with Go 1.26.9 and AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null. The integration tests used real Postgres in testcontainers. No real email, cloud API or purchase was used: the email failure is the unconfigured-notifier path, and the store failures come from mocks or a canceled context.

New and changed tests:

• internal/api/handler_purchases_finalize_test.go (unit, mock store):
  • TestFinalizePurchaseStatus_WriteErrorSurfaces replaces fix(api/purchases): guard the email-failure write in finalizePurchaseStatus on pending #1237's WriteErrorKeepsPending. It checks the 500, the body fields, errors.Is(err, saveErr), that the store error is not in the body, that the caller's copy is not mutated, and that there is no re-read.
  • TestFinalizePurchaseStatus_MissingRowSurfaces: ErrNotFound from the guarded save is also a save failure.
  • TestExecutePurchase_EmailFailureNotRecorded_Returns500 and TestRetryPurchase_EmailFailureNotRecorded_Returns500 drive the full submit and retry handlers. Each one asserts the error and that no success body comes back. They also run the error through handleRequestError, the router's error mapping, and check the status code and body.
  • fix(api/purchases): guard the email-failure write in finalizePurchaseStatus on pending #1237's tests were updated only for the new two-value return.
• internal/api/handler_purchases_finalize_race_integration_test.go (integration, real Postgres):
  • TestFinalizePurchaseStatus_SaveFailureSurfaces inserts a real pending row through persistExecutionAndSuppressions and fails the real sendPurchaseApprovalEmail (no notifier). It then runs finalizePurchaseStatus on a canceled context, so the guarded save fails inside pgx. It asserts the 500, errors.Is(err, context.Canceled), and that the stored row is still pending with an empty error. fix(api/purchases): guard the email-failure write in finalizePurchaseStatus on pending #1237's two integration tests now also require a nil error.

Fail-before / pass-after:

# fail-before: #1237's handler_purchases.go and test files checked out from HEAD (17654ccf),
# plus a temporary file holding only the two new handler tests and their helper
go test ./internal/api/ -count=1 -run 'EmailFailureNotRecorded' -v
# --- FAIL: TestExecutePurchase_EmailFailureNotRecorded_Returns500
#     Expected nil, but got: map[... "email_sent":false ... "status":"pending" ...]
#     An error is expected but got nil.
# --- FAIL: TestRetryPurchase_EmailFailureNotRecorded_Returns500 (same: 200 body with "status":"pending")

# mutation probe on the fix: default branch changed back to `return "pending", nil`
go test -tags integration -count=1 -timeout 20m -run 'TestFinalizePurchaseStatus_SaveFailureSurfaces|EmailFailureNotRecorded|WriteErrorSurfaces|MissingRowSurfaces' ./internal/api/
# all 5 FAIL ("a failed failure write must not be reported as a normal response")

# pass-after at 91827b9a
go test -tags integration -count=1 -timeout 20m -run 'TestFinalizePurchaseStatus|EmailFailureNotRecorded' ./internal/api/ -v
# all PASS, including #1237's ApproveDuringEmailSend and ConcurrentWithApproveClaim (approve claim won 13 of 40 rounds)

The temporary file was deleted afterwards.

Other checks:

go build ./...
go vet ./... && go vet -tags integration ./...
go test ./internal/api/ -count=1                                       # ok
go test ./internal/mocks/ -count=1                                     # ok
go test -short -race -count=1 -run 'TestFinalizePurchaseStatus|EmailFailureNotRecorded|retryPurchase|ExecutePurchase' ./internal/api/   # ok
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/api/...                       # 0 issues
golangci-lint v2.10.1 run --allow-parallel-runners --build-tags integration ./internal/api/...  # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/api/handler_purchases.go   # silent (executePurchase 10, retryPurchase 10)
bash scripts/gosec-hook.sh internal/api/handler_purchases.go            # exit 0
bash scripts/gofmt-hook.sh <changed files>                              # exit 0

This is unit, mock and local integration evidence only. A real SES failure combined with a real database write failure was not exercised.

Combined-tree verification. This branch was tested together with six other queued branches on a local, never-pushed tree: queue/platform @ 77d116c6 = origin/main a14dc8f2 plus these heads, merged in this order:

Branch	Tested head
fix/issue-47	62feb4f
fix/issue-76	3b8c241
fix/issue-1233	e1638b7
fix/issue-153	8729074
fix/issue-532	0971624
fix/issue-64	fefdc43
fix/issue-54	91827b9

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:

• Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks (changed packages): golangci-lint v2.10.1 found 0 issues, with and without --build-tags integration, and gocyclo -over 10 is silent on the changed non-test files. These ran at b61a9c30, which differs from 77d116c6 only by one .gitallowed line on fix/issue-1233.
• Integration tests, every changed package, one package at a time (-tags integration -count=1):
  • These packages pass: cmd/cleanup-lambda, internal/analytics, internal/auth, internal/database/postgres/migrations and internal/purchase.
  • internal/api passes, except two tests whose Postgres container didn't start, or whose migrations hit a timeout, under machine load. Rerun alone on the same tree, both pass: TestDeleteManagedPlanByExecutionStatus and TestNotifiedRow_RescheduleDoesNotOverwriteConcurrentCancel.
  • internal/config passes except TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).
  • Packages were run with a 60-minute timeout (rather than CI's 20) because the VM was shared with other test runs. Some reruns used TESTCONTAINERS_RYUK_DISABLED=true to avoid a testcontainers reaper race under load.
• Secrets: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
• Nothing was applied to any cloud.

Out of scope

• The approval token on the stranded row stays valid until the expiry sweep or a cancel. The 500 names the execution so it can be canceled. Revoking the token automatically would need another write on a path where the store has just failed, so it is not done here.
• The issue's "Sweep target", the log-only audit-field stamp in directExecutePurchase, is unchanged. The issue calls it best-effort attribution, and the 2026-07-28 review tracks it in its grouped internal/api issue.
• The issue body's "the row will expire rather than fire" wording, which the review comment asks to edit, is left for the maintainer.

Frontend follow-up for the UI owner

On this rare path, submit (POST purchase execute) and retry now return HTTP 500 instead of 200. The JSON body is {"error": "AUDIT LOSS: ...", "execution_id": "...", "email_sent": false, "email_reason": "..."}. The UI should already show the error message as a failure. Optionally, when execution_id is present on a 500, link to that execution in History so the user can cancel it.

Maintainers: please mirror the issue's triage labels onto this PR: priority/p3, severity/low, urgency/eventually, impact/internal, effort/s, type/bug, triaged.

🤖 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]

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

Blocking:

• none

Non-blocking:

• Merge order: the diff against main still includes fix(api/purchases): guard the email-failure write in finalizePurchaseStatus on pending #1237's commit 17654cc (the guarded SaveExecutionIfStatus write). I reviewed the combined diff, including that commit, and it's sound: a row lock plus a status check, then the full-row write, and a lost guard re-reads and reports the real status. But if this PR merges first, fix(api/purchases): guard the email-failure write in finalizePurchaseStatus on pending #1237's money-path change lands under this PR's approvals rather than its own. Merge fix(api/purchases): guard the email-failure write in finalizePurchaseStatus on pending #1237 first, as the body says.
• internal/api/handler_purchases.go (retry path): on the 500, the original row already points at the successor, and the successor keeps a valid approval token with no email sent. The message tells the user to cancel it from History, which is reasonable (disclosed). Note that clicking Submit again after the 500 creates a second pending row. That's harmless, since it still needs approval, but it could confuse the user.

Attacked:

• Error mapping: fmt.Errorf("%w: %w", ce, saveErr). I checked that handleRequestError runs IsNotFoundError first, but that matches only the api package's notFoundError type, so a config.ErrNotFound from the save still maps to the structured 500, not a 404 (the MissingRow test drives it through handleRequestError). The store error stays out of the body, and email_reason was already in the old 200 body, so nothing new leaks. The frontend (responseError) shows the specific AUDIT LOSS: message and does not auto-retry a 5xx POST, so there's no duplicate-submit loop.
• Concurrency (fix(api/purchases): guard the email-failure write in finalizePurchaseStatus on pending #1237 part): SaveExecutionIfStatusTx row-locks and checks pending before the write, so a session approve that wins during the email send is never overwritten with failed. Both create paths insert Status: "pending", so the ["pending"] guard covers the window.
• Fail-before: I changed the default branch back to return "pending", nil. TestFinalizePurchaseStatus_WriteErrorSurfaces, ..._MissingRowSurfaces, TestExecutePurchase_EmailFailureNotRecorded_Returns500 and TestRetryPurchase_EmailFailureNotRecorded_Returns500 all fail. Restoring the file makes them pass.
• Ran go build ./..., go vet ./internal/api/ (plain and -tags integration), go test -race -short -count=1 ./internal/api/ and go test -race -tags integration -count=1 ./internal/api/ (testcontainers; includes the canceled-context real-Postgres test and fix(api/purchases): guard the email-failure write in finalizePurchaseStatus on pending #1237's race tests): all ok. The head merges cleanly with current main (35 behind) and with fix(api/purchases): guard the email-failure write in finalizePurchaseStatus on pending #1237's current head 3086bc8.
• Against issue fix(purchase): finalizePurchaseStatus silently swallows save-failure (mirrors COR-10) #54's acceptance: the save failure is no longer reported as pending, the caller gets an AUDIT LOSS 5xx, and a regression test fails against the old behavior. Closes #54 is justified.

Body-stale:

• "This branch is stacked on fix(api/purchases): guard the email-failure write in finalizePurchaseStatus on pending #1237 (fix/issue-1232, head 17654ccf)": fix(api/purchases): guard the email-failure write in finalizePurchaseStatus on pending #1237's head is now 3086bc87. That's only a main merge on top of 17654cc, and it merges cleanly with this branch.

_{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.}