Repository navigation
Conversation
…liation in the notification email Since go#497/LeanerCloud#499 an auto exchange whose accept response was lost stays in AutoExchangeResult.Failed with OutcomeUnknown set. buildExchangeNotificationData dropped the flag, and the completed-email template lists only rows with an empty Error, so the exchange vanished from the email: a run whose only outcome was unknown arrived as "RI Exchanges Completed (1 exchanges)" with nothing listed. Carry OutcomeUnknown into email.RIExchangeItem, render those exchanges first in their own "NEEDS RECONCILIATION - verify in AWS before acting" group with the record ID and the resolve request, keep them out of the completed list, and switch the subject (SNS/SES and SMTP) to "RI Exchanges Need Reconciliation (k of n exchanges)" when any are present. Definite failures keep their current handling. Closes LeanerCloud#1055 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
triaged
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
Since go#497/#499, an auto exchange whose accept response was lost stays in
AutoExchangeResult.FailedwithOutcomeUnknown=true. The record staysprocessing, still counts against the daily cap, and the run halts after it.The platform ignored the flag.
buildExchangeNotificationData(internal/server/handler_ri_exchange.go) copied onlyError. The issue says the exchange was then listed as failed, but on main it is worse than that: the completed-email template (riExchangeCompletedTemplate) lists only rows with an emptyError, so the unknown-outcome exchange disappeared from the email. A run whose only outcome was unknown arrived as "CUDly - RI Exchanges Completed (1 exchanges)" with an empty list, which reads as a success.Fix
email.RIExchangeItemgets anOutcomeUnknownfield, andbuildExchangeNotificationDatacopies it fromexchange.ExchangeOutcome.RIExchangeNotificationData.ReconciliationExchanges()returns the flagged items. The completed template renders them first, in their own group:NEEDS RECONCILIATION - verify in AWS before acting. The group explains that AWS may or may not have executed them, says not to retry them, notes that they keep counting against the daily cap, and gives each exchange's source/target, quoted payment, record ID, thePOST /api/ri-exchange/resolve/<record ID>request (the existing fix(exchange): reconcile stale processing RI exchange rows that now hold daily headroom indefinitely #692 operator-resolve endpoint) and the error detail.OutcomeUnknownrows even whenErroris empty, so the flag alone decides the group.riExchangeCompletedSubject, used by both the SNS/SESSenderand theSMTPSender. With any unknown outcome it readsCUDly - RI Exchanges Need Reconciliation (k of n exchanges). Otherwise it is unchanged (CUDly - RI Exchanges Completed (n exchanges)).Example of the rendered body (from the test fixture):
Closes #1055
Verification
All of this is unit/mock/fixture evidence. No real email was sent: the tests use the existing recording
mockEmailSender(server), a recording SNS publisher stub, and the existing loopbackmockSMTPServer(email). No cloud APIs were called. Environment:AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true, Go 1.26.9.New tests:
internal/server/handler_ri_exchange_reconcile_test.go:TestSendExchangeNotification_OutcomeUnknownRendersReconciliationruns an auto result (completed + unknown outcome + definite failure, the go shape) throughsendExchangeNotification, captures the data and renders the real completed template. The unknown exchange must sit in the reconciliation group before the completed group, and must not appear in either the completed list or (for the definite failure) the reconcile group.internal/email/ri_exchange_reconcile_test.go: render tests (unknown outcome, unknown outcome with emptyError, no unknown outcome means no group), a subject table test, plus wire-level tests forSender.SendRIExchangeCompleted(recorded SNS subject/message) andSMTPSender.SendRIExchangeCompleted(loopback SMTP server:Subject:header and body).Fail-before: the server test compiles on unmodified main and fails there:
(The unknown-outcome exchange
ri-unknownis missing from main's email entirely.) The email-package tests need the new field, so they do not compile on main. Their fail-before evidence comes from the mutation probes below.Pass-after and mutation probes (each mutation applied alone, then reverted):
internal/email/templates.goreverted to origin/main (template and SNS subject)..._OutcomeUnknownNeedsReconciliation,..._OutcomeUnknownWithoutError,TestSender_..._OutcomeUnknown,TestSMTPSender_..._OutcomeUnknownFAIL{{if eq .Error ""}}..._OutcomeUnknownWithoutErrorFAILTestRIExchangeCompletedSubject,TestSender_..._OutcomeUnknown,TestSMTPSender_..._OutcomeUnknownFAILOutcomeUnknown: o.OutcomeUnknownTestSendExchangeNotification_OutcomeUnknownRendersReconciliationFAILCommands run on this branch:
git merge-treeagainst the other open RI-exchange PRs (#1108, #1170, #1191, #1212, #1246, #1249) and against the unopenedfix/issue-326andfix/issue-337branches: all merge clean.Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.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:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_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).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns 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.Out of scope (noticed, not changed)
FailedwithoutOutcomeUnknown) are still dropped from the completed email, as before. That is pinned by the existingTestRenderRIExchangeCompletedEmail_WithError. A run with only hard failures still sends "RI Exchanges Completed (n exchanges)" with an empty list. Listing them as a "Failed" group would be a separate change.buildExchangeNotificationDatanever setsTotalPayment, so both RI exchange emails renderTotal Payment: $with no amount.Exchanges, includingFailedrows. That cannot carry an unknown outcome today, because manual mode never executes.config.RIExchangeResolveStaleAfter(6h), so the email package doesn't depend oninternal/config.Frontend follow-up for the UI owner
None needed for this fix: it changes only the notification email. Optionally, the RI exchange history view could badge
processingrows older than the stale threshold as "needs reconciliation" and link to the resolve action.Maintainer: please mirror the issue's triage labels onto this PR:
triaged,priority/p3,severity/low,urgency/eventually,impact/few,effort/s,type/bug.🤖 Generated with Claude Code