fix: stop retrying INSUFFICIENT_FUND indefinitely

[Quentin-David-24]

Problem

A send stage whose source account cannot cover the amount retries forever.

InfiniteRetryContext (internal/workflow/stages/internal/context.go) guards CreateTransaction and DebitWallet and deliberately sets no MaximumAttempts, so NonRetryableErrorTypes is the only thing that can stop a retry loop. INSUFFICIENT_FUND was not in it — but the ledger just re-evaluates the same postings against an unchanged source balance and rejects them identically on every attempt. The workflow never surfaces the failure to the caller; it just spins.

Fix

Add INSUFFICIENT_FUND to InfiniteRetryContext's NonRetryableErrorTypes.

• The code is spelled identically on both sides — ledger.V2ErrorsEnumInsufficientFund and wallets.ErrorCodeInsufficientFund are both "INSUFFICIENT_FUND" — so this covers both activities the context guards.
• Ledger v2 maps numscript.MissingFundsErr onto the same code (internal/api/v2/controllers_transactions_create.go), so Numscript shortfalls are covered as well as direct postings.
• PaymentInitiationRetryContext is untouched: it guards PSP activities that never return this code, and it is already bounded at 15 attempts.

Tests

• internal/workflow/stages/internal/context_test.go — pins both retry policies. The second test guards against the append() in InfiniteRetryContext mutating the shared commonNonRetryableErrorCodes backing array and leaking ledger-only codes into the PSP context.
• internal/workflow/stages/send/run_test.go — a send case where CreateTransaction returns INSUFFICIENT_FUND, asserting the workflow fails with that code after exactly one activity call. Reverting the one-line policy change fails it with 10 calls and retryable: true, so it genuinely covers the regression.
• This needed a small addition to the shared harness (stagestesting.WorkflowTestCase): ExpectedErrorCode and ExpectedActivityCalls, since it previously only knew how to assert a workflow succeeded. Existing cases are unaffected.

go build ./..., go vet ./internal/... and go test ./internal/... all pass.

Note for reviewers

This is correct for ledger v2, which is what the affected stack runs. Ledger v3 emits INSUFFICIENT_FUNDS (plural), which SDK v5.0.1's V2ErrorsEnum.UnmarshalJSON rejects outright — the SDK then returns a plain unmarshal error rather than a *ledger.V2ErrorResponseError, the activity falls into its default: branch, and the loop comes back through a different route that this change cannot reach. Worth a separate ticket on the SDK or the ledger side if flows is ever pointed at v3.

Deploying this

A Temporal retry policy is captured when the activity is scheduled, not when it runs, so this change only applies to newly scheduled activities. Workflows already wedged in the INSUFFICIENT_FUND loop — exactly the population this fixes — will keep retrying after the deploy and have to be terminated or reset by hand.

Behavioural change worth a second opinion

For a mixed-ledger account to account send, the destination-side throughAccount defaults to world, and the ledger skips the balance check on world, so the common path is unaffected. If someone configures a non-world destination throughAccount and does not fund it, the second transaction (run.go:604) now fails permanently where it previously retried until the bridge account happened to be funded — with the first transaction already committed and no compensation.

I do not think that argues for weakening the fix: allowOverdraft on the destination is the explicit, designed escape hatch for an unfunded bridge account (run.go:578), and a bounded retry would only convert a silent infinite loop into a slow one. But it is a real change for that configuration, so flagging it rather than burying it.

[NumaryBot]

✅ Approve — automated review

The retry policy correctly marks INSUFFICIENT_FUND as non-retryable without mutating the shared policy slice, and the added tests and harness assertions are consistent with the intended behavior.

No findings.

[shipfox-ai]

PR #211 correctly fixes a real production hazard: a send stage from an underfunded source account would retry INSUFFICIENT_FUND forever, because InfiniteRetryContext sets no MaximumAttempts and the code was not in its NonRetryableErrorTypes. The one-line policy change is correct and verified in internal/workflow/stages/internal/context.go:36; PaymentInitiationRetryContext is untouched (still bounded at 15 attempts); the append(append([]string{}, ...)) copy correctly avoids mutating the shared commonNonRetryableErrorCodes backing array and is pinned by a new test; both CreateTransaction (internal/workflow/stages/send/run.go:203,421,487,512,568,604,653) and DebitWallet (send/run.go:272,308,358,372) run under this context, and both activities propagate their SDK error codes as Temporal ApplicationError types (activity_ledger_create_transaction.go:57, activity_wallet_debit.go:60), so the policy applies to both. The harness additions are opt-in and existing cases are unaffected. Two low-severity coverage/verification gaps remain (below); neither blocks the fix.

Recommendation: approve with comments.

Standards

No confirmed material finding. The repo has no documented coding standards, and both review passes' style observations (fixture duplication in run_test.go, string-typed error codes, the new opt-in harness fields) were self-suppressed judgement calls with no correctness, security, compatibility, or test-risk impact, so they are not retained. The append()-aliasing guard is correct, not a smell, and is pinned by TestPaymentInitiationRetryContextKeepsCommonCodes.

Spec

The diff faithfully implements the PR body's spec: the policy line, the unbounded-context rationale, the untouched PSP context, and the new harness fields and tests all check out, with no scope creep. Two low-severity gaps remain:

1. (Low) The DebitWallet half of the coverage claim is untested. The PR states the fix "covers both activities the context guards", but the new case accountToAccountInsufficientFund (internal/workflow/stages/send/run_test.go:777-816) mocks only CreateTransactionActivity. Coverage of DebitWallet is structural only — both activities share the same retry policy — so the wallets error-code propagation through temporal.NewApplicationError(..., string(walletErrorResponse.ErrorCode)) (activity_wallet_debit.go:60) is never pinned by a test. A mirror case mocking DebitWalletActivity would close this; not blocking, since the policy itself is code-shared.

2. (Low) The "INSUFFICIENT_FUND" literal is hardcoded rather than derived from the SDK, and its spelling could not be verified in this checkout. ErrorCodeInsufficientFund = "INSUFFICIENT_FUND" (context.go:16) is asserted in a comment to match ledger.V2ErrorsEnumInsufficientFund and wallets.ErrorCodeInsufficientFund (SDK v5.0.1, go.mod:84), but the SDK is a non-vendored dependency not present in this environment, so the claim rests on the PR author. The new test constructs the error with the PR's own constant, so it pins the retry policy but not SDK interop — if either SDK constant were spelled differently, the fix would silently no-op. Additionally, per the PR's own reviewer note, ledger v3 emits INSUFFICIENT_FUNDS (plural), which fails V2ErrorsEnum unmarshalling and re-enters the loop through a plain retryable error; that divergence is acknowledged as out of scope for this ledger-v2 stack and is tracked only by this PR comment, not a ticket.

Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM.

[shipfox-ai]

This PR adds INSUFFICIENT_FUND to InfiniteRetryContext's NonRetryableErrorTypes, leaves PaymentInitiationRetryContext untouched, and pins both policies plus the new non-retryable behaviour with tests. I verified the change against the diff and the code: the policy line is in place with a proper backing-array copy (internal/workflow/stages/internal/context.go:36-39), the PSP context still uses only the common codes with MaximumAttempts: 15, and the error-code propagation paths that feed the policy (internal/workflow/activities/activity_ledger_create_transaction.go:57, activity_wallet_debit.go:60) are unchanged and reachable from both guarded activities. The new tests exist and match the spec. Recommendation: approve with comments — all findings below are Low and non-blocking.

Standards

No confirmed material finding. The repo has no documented coding standards governing this code (the only CONTRIBUTING.md is for generated SDK clients), so no standard can be violated. The judgement-call smells surfaced during review (fixture overlap in run_test.go, string-typed error codes matching the existing const convention, test-order coupling in context_test.go) all follow established repo conventions and have no correctness, security, compatibility, or test-reliability impact, so they are not retained.

Spec

1. (Low) The DebitWallet half of the fix is untested. The new regression case accountToAccountInsufficientFund (internal/workflow/stages/send/run_test.go:777) mocks only CreateTransactionActivity. DebitWallet runs under InfiniteRetryContext at internal/workflow/stages/send/run.go:272,308,358,372 and propagates wallet error codes via temporal.NewApplicationError(..., string(walletErrorResponse.ErrorCode)) at internal/workflow/activities/activity_wallet_debit.go:60, so it is covered only structurally through the shared policy. A mirror case debiting a wallet with insufficient funds would close the gap. Non-blocking.

2. (Low) The "INSUFFICIENT_FUND" literal is never verified against the actual SDK constants. internal/workflow/stages/internal/context.go:15-16 hardcodes the string with a comment claiming it matches ledger.V2ErrorsEnumInsufficientFund and wallets.ErrorCodeInsufficientFund, but the SDK (formance-sdk-go/v5 v5.0.1) is not vendored or cached in this checkout, so the spelling claim could not be confirmed from source. The test constructs the error from the PR's own constant (run_test.go:809-810), so it pins the retry policy, not SDK interop — if either SDK constant were spelled differently (the PR itself notes ledger v3 emits INSUFFICIENT_FUNDS), the fix would silently no-op. Deriving the constant from the SDK enums, or asserting the propagated code in a test that exercises a real SDK error response, would remove the risk. Non-blocking.

3. (Low) If the fix is reverted, the regression test fails by hanging, not by a clean assertion. With INSUFFICIENT_FUND removed from NonRetryableErrorTypes, the deterministic always-failing mock under an unbounded retry policy means ExecuteWorkflow never returns (backoff is fast-forwarded by the test environment), so the ExpectedErrorCode and ExpectedActivityCalls assertions in internal/workflow/stages/internal/stagestesting/workflow.go:56-65 are never reached — the "exactly one call" pin is only exercised in the passing state, and the revert signal in CI is a hot retry loop until the go-test timeout. This is inherent to testing unbounded-retry behaviour, but noting it so the failure mode is understood. Non-blocking.

No scope creep: every hunk in the diff maps to the spec (policy change, PSP context untouched, policy-pinning tests, harness additions, and the send regression case), and the behavioural effect on non-world overdraft destinations is explicitly flagged in the PR description itself.

Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM.

[shipfox-ai]

This PR adds INSUFFICIENT_FUND to InfiniteRetryContext's NonRetryableErrorTypes so a settled insufficient-funds outcome from CreateTransaction or DebitWallet fails the workflow permanently instead of retrying forever (the context sets no MaximumAttempts), and it pins the change with a new context_test.go, two harness fields (ExpectedErrorCode, ExpectedActivityCalls), and a new table-driven send case asserting the activity runs exactly once. I verified the mechanism end to end: both SDK error paths wrap failures as Temporal ApplicationErrors typed with the upstream error-code string (activity_ledger_create_transaction.go:57, activity_wallet_debit.go:60), so the exact-string policy match applies to both activities; the registered name "CreateTransaction" in the test matches the activity definition; the nested append copy still protects the shared commonNonRetryableErrorCodes backing array; and the harness change preserves the old require.NoError behavior for every pre-existing case. Recommendation: approve with comments — the implementation is correct and minimal; the two findings below are non-blocking robustness/coverage notes.

Standards

No confirmed material finding. Both reviews' candidate smells (repeated require.Contains lines in context_test.go, the map[string]int shape of ExpectedActivityCalls) are style judgement calls with no correctness, security, compatibility, or test-risk impact, and were rejected.

Spec

No formal spec is available; the PR description was used as the reference. All deliverables are present and PaymentInitiationRetryContext is correctly untouched. Two minor findings, both comment-level:

1. internal/workflow/stages/internal/context.go:16 — the INSUFFICIENT_FUND match relies on duplicated string literals with no compile-time link to the SDKs. The new constant is an independent copy of ledger.V2ErrorsEnumInsufficientFund and wallets.ErrorCodeInsufficientFund; Temporal matches ApplicationError.Type() by exact string equality, so if either SDK's spelling drifts (the ledger v3 plural INSUFFICIENT_FUNDS is already acknowledged as uncovered), the fix silently stops matching and unbounded retry loops — the exact failure this PR fixes — return with no test or compile error to catch it. Referencing the SDK enum constants directly (e.g. string(ledger.V2ErrorsEnumInsufficientFund)) or adding a compile-time assertion would close this. Not blocking; the code comment does state the intended pairing today.

2. internal/workflow/stages/send/run_test.go:777 — the wallet (DebitWallet) path is untested. The new case covers only the CreateTransaction (account-to-account) path; the claim that the fix covers both guarded activities rests on code inspection of activity_wallet_debit.go:60 rather than a test. A wallet-source send case asserting ExpectedErrorCode: internal.ErrorCodeInsufficientFund and a single DebitWallet call would pin the second half of the claim. Not blocking.

Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM.