Skip to content

fix(linq): retry a NIN check the provider failed, never one it answered - #262

Merged
Miracle656 merged 2 commits into
Miracle656:mainfrom
JemimahEkong:fix/w079-kyc-retry-on-provider-failure
Oct 20, 2026
Merged

Miracle656 merged 2 commits into
Miracle656:mainfrom
JemimahEkong:fix/w079-kyc-retry-on-provider-failure

Conversation

@JemimahEkong

Copy link
Copy Markdown
Contributor

W079 · Retry a NIN check the provider failed, never one it answered

Closes #236

Problem

provisionCustomer and createOnrampOrder wrap their provider call in retryOnProviderFailure. submitCustomerKyc called straight through, so a single transient 5xx from the identity provider was final.

That is the least forgiving place to have no retry. A NIN verifies once, ever, against one customer reference, and a person who meets a blip can't tell it from a refusal. Nor can the screen behind them. On 2026-10-14 the endpoint returned a 5xx in about 2.2 seconds, consistently, and every user hit it bare.

What changed

All in src/linq/onramp.ts.

Which failures are retried (isRetryableKycFailure):

Failure Retried? Why
Provider 5xx Yes The provider answered, and the answer was that it failed
Connection that got no answer Yes Nothing came back
Any 4xx: nin_already_used, invalid_nin_format, nin_verification_failed, 429 Never These are answers. Each retry spends an attempt against a rate-limited provider and changes nothing
Our own timeout No The provider may still be working on the NIN we sent, so resending is exactly the duplicate attempt this must never make

Our own timeout and a provider's 504 are both status 504, so the status alone can't tell them apart. Transport failures are therefore now tagged timeout or unreachable where they are thrown.

The NIN is never resent to a route that has already answered. A failure can hide a success: the provider may have verified the customer and then failed to say so. Before any resend, the code calls GET /b2b/customers/status, which takes the customer reference alone and no NIN.

  • If the customer is already verified, that is the answer and the NIN does not travel again.
  • If the status can't be read either, the original failure is returned rather than resending blind.

Backoff and budget.

  • The waits are 1s and then 2s, with at most three attempts.
  • One deadline, KYC_BUDGET_MS = 35s, bounds every attempt, wait and status check. It is under the clients' own 40s ceiling, so nobody is told the check failed while this server is still running it.
  • Each attempt gets min(30s, what is left), and a wait that would end past the deadline isn't taken.

A fix on the way. submitCustomerKyc used to pass timeoutMs: 30_000 with no deadline. In that case the onramp client silently capped the timeout at its 15s default, so the 30s added for NIMC's slow answers never applied. Passing the deadline makes it apply.

docs/ngn-rails.md said the call was "deliberately not retried". It now describes the policy above.

Acceptance criteria

  • A 500 followed by a 200 verifies, with one retry and no duplicate side effect. Tested: exactly two KYC posts with an identical body, one status check between them, and the NIN never in the status request. The same is tested through POST /ngn/customers/kyc.
  • A 409 nin_already_used is returned immediately, with no retry. Tested for 409, 400 invalid_nin_format and 422: one post, no status check, no backoff slept. A 429 is still not retried.
  • The total elapsed time is bounded and tested, not just the per-attempt timeout. Tested under fake timers:
    • A provider that fails slowly (12s per attempt): 12 + 1 + 12 + 2 = 27s, and the third attempt is cut off at the 35s deadline instead of getting a fresh 30s.
    • A failure late enough that the next wait would cross the deadline: no further attempt is made.
    • Three fast failures: the operation gives up well inside the budget.
    • A provider that never answers: 504 at 30s, not retried.
  • The retry does not resend the NIN to a route that has already answered. Tested in three cases:
    • status says verified: one post, success returned;
    • status unreadable: one post, original error returned;
    • our own timeout: one post, no status check.

Validation

  • npm test: 735 of 736 passing. The single failure is the pre-existing openapiCoverage test (get /fees undocumented), which is W082's job and is unchanged by this PR.
    • The 25 new tests are in src/__tests__/linq/kycRetry.test.ts.
    • No existing test file was modified. The existing suites' 500 and 502 KYC cases still pass, because their status check fails too, so the original error comes back unchanged.
  • npm run typecheck, npx tsc --noEmit and npm run build all pass.
  • I mutation-checked the suite by breaking the code three ways: ignoring a verified status, dropping the shared deadline, and retrying a 409. Each broke tests (1, 3 and 2 failures respectively).
  • Every test stubs global.fetch; nothing can reach the provider. The only NIN-shaped value used is 00000000000.

Note for reviewers

This PR and the W078 PR (one HTTP client for Linq) both change src/linq/onramp.ts, so whichever merges second needs a rebase. If W078 lands first, two pieces of this change move into src/linq/http.ts:

  • the transport tag;
  • the deadline passed by the KYC call.

The retry policy itself stays in onramp.ts.

…ed (W079)

submitCustomerKyc called straight through, so one transient 5xx from the
identity provider was final. A NIN verifies once, ever, against one customer
reference, and on 2026-10-14 the endpoint returned a 5xx in ~2.2s for
everyone; nobody could tell that blip from a refusal.

- Retried: a provider 5xx, or a connection that got no answer. Backoff 1s
  then 2s, at most three attempts.
- Never retried: any 4xx (nin_already_used, invalid_nin_format, a 429 —
  answers, and each retry spends a rate-limited attempt), and our own
  timeout, because the provider may still be working on the NIN we sent.
- Before any resend, GET /b2b/customers/status (reference only, no NIN). A
  customer already verified is the answer and the NIN is not sent again; a
  status that cannot be read means no blind resend.
- One 35s deadline bounds every attempt, wait and status check — under the
  clients' 40s ceiling. Passing it also lets the 30s per-attempt timeout
  apply; without a deadline it was silently capped at 15s.

Transport failures are now tagged (timeout | unreachable) so our own abort
can be told apart from a provider's 504.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LvL6Yz2Zfg7spM1F9DXXtz
@drips-wave

drips-wave Bot commented Oct 17, 2026

Copy link
Copy Markdown

@JemimahEkong Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@Miracle656 Miracle656 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving on substance. It needs to go in after #261, which consolidates the two Linq HTTP clients and conflicts with this — I will tell you when to rebase, and nothing else is queued between them.

This is a careful piece of work on the one call in the service where being careless is unrecoverable.

The title is the design. "Retry a NIN check the provider failed, never one it answered." A NIN verifies once, ever, against one customer reference — so a retry that reaches an endpoint which already answered does not cost a round trip, it costs the person their verification.

The timeout decision is the one most people get wrong, and you got it right:

if (transport === "timeout") return false;
if (transport === "unreachable") return true;
return err.status >= 500;

Not for our own timeout. The provider may still be working on the NIN we sent, so sending it again is exactly the duplicate attempt this must never make.

A timeout is ambiguous, not a failure — treating it as "may have been answered" is the conservative read and the correct one. unreachable is the opposite: the request never arrived, so nothing was spent. Distinguishing those two is the whole safety argument, and most implementations collapse them into "network error, retry".

Asking before resending is better than anything I would have specified:

A failure can hide a success: the provider may have verified the customer and then failed to tell us. Before any resend we ask GET /b2b/customers/status.

That endpoint takes the reference alone — no NIN — which is exactly why it exists. If the first attempt landed and the response was lost, the status call finds it and the NIN never travels twice. If even that cannot be answered, you stop. That is the right failure.

The observed data backs the issue too: a 5xx in about 2.2 seconds, consistently, on 2026-10-14, with every user meeting it bare. That is the shape of a provider blip, not a refusal, and it was reaching people as "your identity could not be verified".

src/__tests__/linq: 143 passed. The NIN-never-escapes assertions still hold.

Rebase on main once #261 lands — it is approved and waiting on CI. Ping me and I will merge this straight after.

@Miracle656

Copy link
Copy Markdown
Owner

#261 has landed — the two Linq HTTP clients are now one src/linq/http.ts. Please rebase on current main and ping me; this is approved and I will merge it straight away.

Your isRetryableKycFailure will need to read transport off the error the consolidated client throws rather than the old one, but the shape should be the same — #261 kept LinqError and its fields, including the existingCustomerRef I added this morning.

Note Integration tests is red on main (tests/integration/api.test.ts, Postgres read path, failing since at least 10-16). Not yours, and not a reason to hold your rebase.

Brings the branch onto Miracle656#261's single Linq client (src/linq/http.ts).

Conflict in src/linq/onramp.ts: this branch tagged timeout/unreachable
inside onramp's private call(), which Miracle656#261 removed. The tagging now lives
in the shared client, as requested in review: http.ts marks the failures
it raises itself (`transport: "timeout" | "unreachable"`, including the
deadline-exhausted 504) and exports linqTransport(err).
isRetryableKycFailure reads it from there. LinqError's fields are
unchanged, and http.ts remains the only module naming the base URL,
host or X-API-Key.

Verified: typecheck, tsc --noEmit, build; jest 63 suites / 933 tests.
Removing the timeout tag fails kycRetry ("never resend after our own
timeout").

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LvL6Yz2Zfg7spM1F9DXXtz
@Miracle656
Miracle656 merged commit f594a2b into Miracle656:main Oct 20, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A NIN check that fails once fails for good

2 participants