Skip to content

test(ngn): prove the idempotency claims, and correct the ones that do not hold (W090) - #249

Merged
Miracle656 merged 2 commits into
Miracle656:mainfrom
Glorious21:test/w090-idempotency-proven
Oct 20, 2026
Merged

Miracle656 merged 2 commits into
Miracle656:mainfrom
Glorious21:test/w090-idempotency-proven

Conversation

@Glorious21

Copy link
Copy Markdown
Contributor

Summary

Proves (or corrects) every idempotency claim on the naira rails.

  • Fake provider (src/__tests__/fakes/linqProvider.ts): replaces global.fetch with no way through to the network; each provider assumption is marked BELIEF:. W089's fake doesn't exist yet, so this one is kept small for W089 to extend. No KYC endpoint, so it never sees a NIN.
  • Tests: linq/idempotency.test.ts (client functions) and routes/idempotency.test.ts (HTTP routes, with an in-memory DB that enforces the schema's unique indexes). They cover a plain duplicate and three kinds of dropped response: the client never got our answer; Linq's answer never reached us; Linq created the order but our DB write failed.
  • docs/idempotency.md: per-route guarantees, the key's scope and lifetime, and what is proven versus believed.

Claims corrected

  • retryOnProviderFailure said every retry carries an idempotencyKey. Onramp and bills send none, so a retry after a lost answer creates a second (unpaid) order. Now documented and pinned in tests; behaviour unchanged.
  • provisionCustomer: keyed on customerRef alone, so a repeat with new details keeps the old ones, and created can be false for a customer the same call created.
  • Webhook: idempotent per event, but a late redelivery of an earlier event overwrites the status.
  • recordNgnOrder: "a double-submit updates the row" was wrong. A double-submit is two orders.
  • Offramp: if our DB write fails or two identical requests arrive together, only Linq's dedupe on the key prevents a second order, and Linq hasn't documented that. A concurrent duplicate makes one order; the losing request gets a 500, and retrying it replays the order.

For W079

Before retrying KYC, ask Linq what a second submission of the same NIN for the same customerRef returns. If it is nin_already_used, a retry after a dropped response turns a success into a refusal.

Related issue

Closes #247

Type of change

  • Bug fix
  • New feature
  • Refactor
  • Docs
  • Tests
  • CI / tooling

Checklist

  • I have read CONTRIBUTING.md
  • npx tsc --noEmit passes
  • npm run build passes
  • I added / updated tests where relevant
  • I updated docs where relevant

npm test: 719 passed, 2 failed, both in openapiCoverage (W082, red on main already). Checked that the tests aren't empty: switching off the fake's offramp dedupe fails 6, and bypassing the route's own key check fails 2.

🤖 Generated with Claude Code

… not hold (W090)

Adds a fake Linq provider that records each provider assumption as a
labelled BELIEF, plus client-level and route-level suites covering a
duplicate request and three kinds of dropped response.

Corrects comments that overclaimed: onramp and bill creation are
retried without an idempotency key, provisionCustomer is keyed on
customerRef alone, and the webhook is idempotent per event but not
order-safe. docs/idempotency.md records each route's guarantee and the
key's scope and lifetime.

Closes Miracle656#247

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@drips-wave

drips-wave Bot commented Oct 15, 2026

Copy link
Copy Markdown

@Glorious21 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

Keeps both doc comments on provisionCustomer: the reused-reference note from main and the
caveats about what idempotent provisioning does not promise.
@Miracle656
Miracle656 merged commit 8ec6a6e into Miracle656:main Oct 20, 2026
2 of 4 checks passed
@Miracle656

Copy link
Copy Markdown
Owner

Merged. The finding is worth more than the tests, and the tests are good.

  • offramp orders carry an idempotencyKey, and Linq is believed to return the original order for a key it has seen. Believed, not documented to us.
  • customers are keyed on customerRef, and Linq returns the existing one.
  • onramp orders and bills carry NO key. A retry here after a lost answer places a second, unpaid order; the caller is given the second.

Three different guarantees behind one retryOnProviderFailure, and "believed, not documented to us" is exactly the right register for the first — we are relying on provider behaviour nobody has given us in writing, and saying so is more useful than asserting it.

src/linq/http.ts claimed the opposite, and I've replaced it with your account. It read:

The retry sends the identical body, idempotencyKey included, so the provider can recognise it as the same order.

True for the offramp, false for onramp orders and bills. That is precisely the sentence that would let the next person wrap a fourth unkeyed call in the same helper on the grounds that it is safe. A comment that is confidently wrong about money is worse than no comment, and finding it is the kind of thing only writing the tests would have surfaced.

Two resolutions while merging

src/linq/client.ts conflicted because your branch re-adds retryOnProviderFailure there, where it lived before #261 consolidated the transport into src/linq/http.ts. I kept main's layout and moved your improved documentation to where the function now lives — the words were the contribution, not the location.

src/__tests__/routes/idempotency.test.ts asserted that the loser of a concurrent duplicate receives a 500:

expect([a.status, b.status].sort()).toEqual([201, 500]);

pinned as a known edge, which was correct when you wrote it. #210 closed that edge earlier today: the route catches the unique violation, reads the winning row back and answers it through the same replay path a sequential duplicate takes, so the loser now gets 200 and the order. I updated the assertion and the test name and left the history in the comment.

I want to be clear that this is your suite working, not failing. It noticed a deliberate behaviour change on the day it landed, which is the entire point of pinning behaviour — and it is a much better outcome than the test having been written loosely enough not to notice.

Verified on the merge result rather than your branch: tsc --noEmit silent, 1,025 tests across 68 suites, nothing red.

Closes #190.

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.

Idempotency is claimed in comments and nowhere proven

2 participants