Repository navigation
fix(purchase): finalizePurchaseStatus silently swallows save-failure (mirrors COR-10) #54
Description
Problem
handler_purchases.go:1594 finalizePurchaseStatus mirrors the COR-10 (LeanerCloud/cloud-commitments-cli#1184 / PR LeanerCloud/cloud-commitments-cli#1237) anti-pattern. When the approval email fails to send, it stamps Status="failed" on the row and calls SavePurchaseExecution. If that save fails, the function logs an error and returns "pending" to the API caller:
execution.Status = "failed"
execution.Error = emailReason
if err := h.config.SavePurchaseExecution(ctx, execution); err != nil {
logging.Errorf("failed to mark execution %s as failed after email send error: %v", execution.ExecutionID, err)
return "pending"
}
return "failed"The row remains in whatever its previous status was (typically pending) and the API call appears successful to the caller, while:
- the failed-email reason is not persisted onto the row,
- the dashboard/history UI sees no signal that the approval email failed,
- the row sits in
pendinguntil the History expiry sweep (LeanerCloud/cloud-commitments-cli#1158) reaps it, with no breadcrumb explaining why.
This is not on the money path: the email never went out so no human approval link can be clicked, and the row will expire rather than fire. But it is the same silent-failure shape COR-10 closed on the credential-resolution path, so the audit-trail loss is identical.
Suggested fix
Mirror PR LeanerCloud/cloud-commitments-cli#1237: bubble the save error so the caller can act on it (or at minimum return "failed_send_audit_loss" instead of "pending", plus an explicit error response). The two known shapes are:
- Return the save error from
finalizePurchaseStatus. Change the signature to(string, error)and surface anAUDIT LOSS:error to the HTTP handler so the response carries a 5xx with the audit-loss reason. Callers athandler_purchases.go:1120and:1927would propagate the error to the existing error path. - Persist via the atomic
TransitionExecutionStatus(["pending","notified"], "failed", ...)CAS. This is whatapprovals.go/manager.gouse elsewhere and gives an atomic guard against a concurrent approve clobbering the failed-state write.
Either approach removes the silent-failure window. (1) is the minimal change; (2) aligns the email-send-failed path with the rest of the failed-status state machine.
Sweep target
This file (internal/api/handler_purchases.go) and handler_purchases.go:2004 (direct-execute audit-fields stamp) are the two remaining log-only SavePurchaseExecution error sites in the handler_purchases package. The latter is best-effort attribution; this one is a real status-write.
References
- PR fix(purchase): surface save error on failed-status execution record cloud-commitments-cli#1237 (the credential-resolution sibling, just fixed).
- Issue COR-10: Failed-status purchase execution save is unchecked (_ =) while the success-path save treats failure as audit loss cloud-commitments-cli#1184 (COR-10) for the underlying audit-loss requirement.
Acceptance
finalizePurchaseStatus's save-failure no longer returns"pending"silently; the API caller sees either an error or afailed_send_audit_lossstatus.- Regression test that asserts the new contract: with
SavePurchaseExecutionreturning an error, the response carries the save-failure (and the test fails against the current code).
Activity
cristim commentedon Jul 28, 2026
Reviewed commit:
be11bdcb5. Note:origin/mainmoved to3e9660d06during the review; re-verify against currentmainbefore changing code, since a finding may have been fixed or moved.Re-derived independently during the 2026-07-28 full review of
internal/apiand confirmed still live onbe11bdcb5. Same defect, same function. Recording as evidence, but contesting the severity rationale, because the review found two consequences this issue's write-up explicitly rules out.Where, on current line numbers:
internal/api/handler_purchases.go:2218-2229, called fromexecutePurchase:2558andretryPurchase:1642.This issue states:
Both halves of that are weaker than they look on
be11bdcb5:1. The row still holds a live approval token. The failed save means the row keeps
status = pendingwith an emptyerrorcolumn and its approval token intact and valid until the 7-dayapprovalExpiryWindowsweep. The premise "the email never went out, so nobody can click the link" holds only if the send failure was total and no token was ever transmitted or logged anywhere. Anyone who does hold that token can approve a real purchase whose requester was told the request was merely "pending" delivery. "The row will expire rather than fire" is the best case, not the guaranteed one.2. The returned status matches neither the in-memory object nor the DB row. The function sets
execution.Status = "failed"in memory, fails to persist it, and then returns"pending". The API response is{"status":"pending","email_sent":false}. The persisted row is a normal-lookingpendingexecution with no failure reason, so the History view offers nothing to explain it andresolveOpsHint(handler_purchases.go:1560) has nothing to match against. The operator's only signal is a server-side log line.Failure scenario as re-derived: SES is misconfigured and the
SavePurchaseExecutionwrite races a connection reset. The requester seespending, no approval email exists, the row is indistinguishable from a healthy pending execution, and it sits with a live token until the 7-day sweep expires it.On the fix: either of the two shapes already sketched in this issue closes it. The review's phrasing of the requirement: never report the pre-mutation status after a failed state write; return an explicit error or a distinct
unknown/failed_send_audit_lossstatus. If option (1) (bubble the error) is chosen, the response should be a 5xx so the caller retries or alerts rather than recording a phantom pending.Triage note: currently
priority/p3/severity/low/impact/internal, justified by the "not on the money path" reading above. If the live-token point stands, that reading is too generous and this deserves a re-look. At minimum the "will expire rather than fire" claim should be edited out of the issue body so the next reader does not inherit it.Related: LeanerCloud/cloud-commitments-cli#1310 (silent save-failure after an irreversible Execute). This issue's own "Sweep target" section names
directExecutePurchase's audit-stamp site as the sibling; the 2026-07-28 review records that one in the grouped medium/low issue forinternal/api, since it is pre-execute and therefore cleanly recoverable with a 500.