From 9040be39089b2786acb1fa6ecceca902e2c28f63 Mon Sep 17 00:00:00 2001 From: Leo Cagliero Date: Fri, 21 Aug 2026 20:34:09 -0300 Subject: [PATCH] feat(transfers): make transfer creation idempotent under retries createTransfer submitted the payment to the provider before persisting anything, so a client or provider retry that arrived after a timeout found no record of the first attempt and moved money a second time. Each attempt also wrote its own audit entry, which made the duplicate hard to see afterwards. The fix reserves the (actor, key) pair before any side effect runs, and stores the terminal result once the transfer exists: begin -> reserve, replay, or refuse provider call and persistence complete -> store the result for later retries release -> on failure, so a correct retry can still win Design notes: - Scoped by API token. Keys are chosen by clients, so a bare namespace lets one caller collide with, or probe for, another operation that is not theirs. - The fingerprint covers the fields that determine the transfer, not the raw body, and is canonicalized: property order, an amount sent as a string and unrelated extra fields do not turn a legitimate retry into a 409. - Conflicting reuse is checked before in-flight state, so a client reusing a key for a different operation gets the same clear answer either way instead of being sent into a retry loop that can never win. - A replay short-circuits before the quote is recomputed. Rates move, so recomputing would hand back a different transfer under the same key. - A provider failure releases the reservation. Burning the key would be worse than the duplicate it prevents. Compatibility: the header is now required on POST /api/transfers, which is a breaking change for clients that omit it. That is deliberate on a money path: a client with no key is not opting out of protection, it is unaware it needs it. The service entry point keeps the context optional because idempotency is actor-scoped and internal callers have no actor to scope to; the route supplies it on every request-driven creation. Tests: 28 new (19 service, 9 HTTP) covering retry, conflict, concurrency, provider failure and recovery, restart, actor scoping and fingerprint canonicalization. The concurrency case re-enters from inside the provider call, which reproduces the exact window the bug lived in rather than simulating it. Suite goes 153 to 181, all passing. Two existing test files were updated to send the header; no assertion was relaxed. Closes #128 Co-Authored-By: Claude Opus 5 --- README.md | 29 +++ src/controllers/transferController.js | 59 ++++- src/services/idempotencyService.js | Bin 0 -> 5645 bytes src/services/transferService.js | 59 ++++- src/store/index.js | 5 + test/moneyPrecision.test.js | 2 + test/requireScope.test.js | 14 +- test/transferIdempotency.test.js | 345 ++++++++++++++++++++++++++ test/transferIdempotencyHttp.test.js | 149 +++++++++++ 9 files changed, 653 insertions(+), 9 deletions(-) create mode 100755 src/services/idempotencyService.js create mode 100755 test/transferIdempotency.test.js create mode 100755 test/transferIdempotencyHttp.test.js diff --git a/README.md b/README.md index ac8013d..925be9a 100644 --- a/README.md +++ b/README.md @@ -159,6 +159,7 @@ rounded away. - `POST /api/transfers` — create a transfer. Body: `{ senderName, recipientName, amount, from, to }` + Requires an `Idempotency-Key` header (see below). - `GET /api/transfers` — list transfers. Supports `?status=`, `?q=` (name search), `?archived=` (true/false/all), and `?limit=`/`?offset=` pagination. Archived transfers are excluded from results by default. @@ -169,6 +170,34 @@ rounded away. - `POST /api/transfers/:id/archive` — archive a transfer, hiding it from default list results. - `POST /api/transfers/:id/unarchive` — unarchive a transfer, restoring it to default list results. +#### Idempotency + +`POST /api/transfers` requires an `Idempotency-Key` header. The endpoint moves +money, so a client that omits the header is not opting out of protection, it is +unaware it needs it; the request is rejected with 400 rather than risking a +duplicate. + +For a given (API token, key) pair the operation runs at most once: + +- **Retry with the same payload** replays the stored transfer, answering 201 + with the original body. The quote is not recomputed, so a moved rate cannot + change the answer, and no second payment is submitted. +- **Reuse with a different payload** answers 409. The fingerprint covers the + fields that determine the transfer, so property order, an amount sent as a + string, and unrelated extra fields all still count as the same request. +- **A retry arriving while the first is still in flight** answers 409 rather + than starting a second settlement. +- **A provider failure releases the key**, so retrying with the same key can + succeed once the provider recovers. + +Keys are scoped to the API token: two callers using the same key get two +independent transfers, and neither can reach the other's. + +Records live in the same store as the transfers, so with the in-memory store +that backs this demo a restart clears both together. That keeps them +consistent: a surviving reservation would replay a transfer that no longer +exists. + ### Users - `GET /api/users` — list users. diff --git a/src/controllers/transferController.js b/src/controllers/transferController.js index 5815bf7..e1ef2f4 100644 --- a/src/controllers/transferController.js +++ b/src/controllers/transferController.js @@ -1,8 +1,41 @@ 'use strict'; const transferService = require('../services/transferService'); +const idempotencyService = require('../services/idempotencyService'); +const ApiError = require('../utils/ApiError'); const { parsePagination } = require('../utils/pagination'); +/** Upper bound on a client-supplied key, so the map cannot be grown without limit. */ +const MAX_IDEMPOTENCY_KEY_LENGTH = 255; + +/** + * Read and validate the Idempotency-Key header. + * + * Required rather than optional: this endpoint moves money, and a client that + * omits the header is not opting out of protection, it is unaware it needs it. + * Failing the request is the only outcome that cannot silently duplicate a + * transfer. + * + * @param {import('express').Request} req + * @returns {string} + * @throws {ApiError} 400 when the header is missing or unusable. + */ +function requireIdempotencyKey(req) { + const raw = req.get('Idempotency-Key'); + if (typeof raw !== 'string' || raw.trim() === '') { + throw ApiError.badRequest( + 'Idempotency-Key header is required to create a transfer' + ); + } + const key = raw.trim(); + if (key.length > MAX_IDEMPOTENCY_KEY_LENGTH) { + throw ApiError.badRequest( + `Idempotency-Key must be at most ${MAX_IDEMPOTENCY_KEY_LENGTH} characters` + ); + } + return key; +} + /** * Transfer controllers. */ @@ -12,7 +45,31 @@ const { parsePagination } = require('../utils/pagination'); * Create a new transfer. */ function createTransfer(req, res) { - const transfer = transferService.createTransfer(req.body, req.id); + const key = requireIdempotencyKey(req); + + // The fingerprint covers the fields that determine the operation, not the raw + // body: an unrelated extra property must not read as a conflicting retry. + // `amount` is normalized because "100" and 100 both validate and produce the + // same transfer, so treating them as different requests would reject a + // legitimate retry from a client that re-serialized its payload. + const fingerprint = idempotencyService.fingerprint({ + senderName: req.body.senderName, + recipientName: req.body.recipientName, + amount: Number(req.body.amount), + from: req.body.from, + to: req.body.to, + }); + + const transfer = transferService.createTransfer(req.body, req.id, { + actor: req.token, + key, + fingerprint, + }); + + // A replay answers 201 with the original transfer, exactly as the first call + // did. Replaying the stored result means replaying all of it; downgrading the + // status would make a successful retry look different from the response it is + // standing in for. res.status(201).json(transfer); } diff --git a/src/services/idempotencyService.js b/src/services/idempotencyService.js new file mode 100755 index 0000000000000000000000000000000000000000..40b50a6f68d2ffc2da2ee0db733001be3fa47312 GIT binary patch literal 5645 zcmb_gU31&U70t7L#kIy0k{$`_Ltjc)XBfH5={bCB(y?Z~-J$o)DeQTxd8kcvImnSE=uiH*uZQPrt@r_(cV}I{mW2ciW zyXZStwioH;WQO$@FJ7F;3wf1yzBzAmzp;h5!d4sK**f3J5=N@NGo7nfYRk4=n_&xg zy<1yZ^`fl8@mK$&yU+-wJG@UN|y@%V9 z!=D}F=+-bx`(hn-)6}LkyI`t?3rGSsR<1TB-nD%R2O1h&8e~vP-xuXBldDb&4-aaO zAGYW>rOR=mEOxNIE4?Y^(rvvIZn?CLtvg26$`*&hPufGO>JFPqS@>cnra>CH!Io~p zU`Ii>XR=+pe9g>m8Gwumr#isqk8buylfbsnCOeZ*A_I0+|O?#ML3BHBK%7D%4ATff1Cn z=o_vpucsY?sks zV3MmJfBpLVt8c!22P&Dk`qyRYR%R~blMNz8 z1U|X^c);uHAAb7&&AT^W@qpY{o6>f+AeBXG`e*MDD(&#CZykKBP#-`pliSa3nJqS; z*=$e{f&OHrgWJ+1&w)KAZ@0&V6$G{6W3jfA;(VCik~5PxeEW zZ*1dC>HY(f2nh?``^9fIkF?c-!;u_xh4L#JV$-c-@E&LYl!UsV zmOMNMjqCqZme@_E3g|m6ILs0YV2C} zWnlhDO|S`%8oN;JuxC4T^|JAm7#c(qtQACCSC(pZ6Pg$#(7i;b*>F**88i&2*4KbO zQjt23d7-J&u3YCpm&7?LX_d2v;`a4j?YH$lB$XS!{B#+CJ6|(8)MBd8oe7KRmJ7hSGltf`R-k1l zcpDGELXnK1o6f>-TAtp`DFHbTKz|y&Fb%H8&9~H5(QdH!X}pYl znnpK5X=1bo8A%Jqo&UNtuF_Kvh(qPk!h!-p3H44|-rg2E20hU9_2_2i<%h%|ga#rD zUCN|wLI03ts66gAK0t%rJKwkHBpA}GKCpU{Zlq6bcCrO_)Us_LLN&|0{G;4JSZL+Y zm`wvYQ1CM6`TA_@>H?Z`38ACbC@uJsvas@8T|t#V<$ybA6S{qc9!i2FnV755HMRhs*3+2mc;D(oz!4_x93 zopxBH?+!0Pa!%x%_mEqJ1LqEgl0UWsm? z{R1ShPob=2A2=B9UR=oQc&{-jT4xpLQri8*Ff}$09Rf(DD*l9}0)*81o<^fD^ zxFuRS4JZRUQ4^f1Gdd|bND>-EdMO%)02ci!$Oeu{>Rkn$0+pH+L+O1S1VE!vl7nFY zS-p=FRI*Xcg}3m>kSYj{BXR1t46syLF@;6`n_NBe$~j<+Ne=3xgyWbFpYFocVzC=1 z2_X;TIUc5Tl+!C+B;M}FVT1vI`eHzJ>Vp5}_p20hpb4Frj$;9`LWP1BT%iTpKk06~ z)DCcc#Bf7VLWdFVBbmz)P`PcGdg3z72I*@-&?MoTfDR1!q=EwQa&GK-gx3MZf9k>* zUo3z2Do7e4_;^VF_uds`@ey~X@#$mq$tz$?nSle3$S^!SqEH14CFry8I>92;BZ<23 z4my)+_)><^j_>2Um~FNQy&1hp&V&B2_hbAwF6LW{5gcPByLFhrsXXxPk$CvY=8?2oit=_z;jjHz*O_Sv;aRQGSdJHtG*$G0t1GhW~l0?^kOwrN)7#8ZFQ834lo< zEwkOsQk7TkT7DK<*xc}Qp)xGSs$qN&9~*`Wq^~Jx6&S&QeB3OcNaUqML@8})nEk(Z zLO>Wa$`g2R0sx+Z`xxebL>UBl9<_q%fn2 ". Lives here rather than in a module + // local so it shares the transfers' lifetime: a replay can never outlive the + // transfer it would replay. + idempotency: new Map(), }; /** Remove all records from the store. Primarily used in tests/seeding. */ function reset() { store.users.clear(); store.transfers.clear(); + store.idempotency.clear(); auditService.reset(); } diff --git a/test/moneyPrecision.test.js b/test/moneyPrecision.test.js index 2814992..caccb6a 100644 --- a/test/moneyPrecision.test.js +++ b/test/moneyPrecision.test.js @@ -67,6 +67,7 @@ test('POST /api/transfers rejects an amount with sub-cent precision', async () = headers: { Authorization: 'Bearer test-token-admin', 'Content-Type': 'application/json', + 'Idempotency-Key': 'idem-precision-1', }, body: JSON.stringify({ senderName: 'Alice', @@ -88,6 +89,7 @@ test('POST /api/transfers accepts a well-formed two-decimal amount', async () => headers: { Authorization: 'Bearer test-token-admin', 'Content-Type': 'application/json', + 'Idempotency-Key': 'idem-precision-2', }, body: JSON.stringify({ senderName: 'Alice', diff --git a/test/requireScope.test.js b/test/requireScope.test.js index 41868fc..33b723d 100644 --- a/test/requireScope.test.js +++ b/test/requireScope.test.js @@ -116,7 +116,7 @@ test('GET /api/users returns 401 when token is unknown', async () => { test('POST /api/transfers returns 401 when token is unknown', async () => { const { status, body } = await fetchJson('/api/transfers', { method: 'POST', - headers: { ...authHeader('bad-token'), 'Content-Type': 'application/json' }, + headers: { ...authHeader('bad-token'), 'Content-Type': 'application/json', 'Idempotency-Key': 'idem-requireScope-118' }, body: JSON.stringify({ senderName: 'Alice', recipientName: 'Bob', amount: 100, from: 'USD', to: 'EUR' }), }); assert.equal(status, 401); @@ -129,7 +129,7 @@ test('POST /api/transfers returns 403 when token only has transfers:read scope', // test-token-readonly has: transfers:read, users:read, audit:read — no :write scopes const { status, body } = await fetchJson('/api/transfers', { method: 'POST', - headers: { ...authHeader('test-token-readonly'), 'Content-Type': 'application/json' }, + headers: { ...authHeader('test-token-readonly'), 'Content-Type': 'application/json', 'Idempotency-Key': 'idem-requireScope-131' }, body: JSON.stringify({ senderName: 'Alice', recipientName: 'Bob', amount: 100, from: 'USD', to: 'EUR' }), }); assert.equal(status, 403); @@ -153,7 +153,7 @@ test('POST /api/transfers/:id/claim returns 403 when token only has transfers:re // Create a transfer first using the admin token, then try to claim with read-only const createRes = await fetchJson('/api/transfers', { method: 'POST', - headers: { ...authHeader('test-token-admin'), 'Content-Type': 'application/json' }, + headers: { ...authHeader('test-token-admin'), 'Content-Type': 'application/json', 'Idempotency-Key': 'idem-requireScope-155' }, body: JSON.stringify({ senderName: 'Alice', recipientName: 'Bob', amount: 100, from: 'USD', to: 'EUR' }), }); assert.equal(createRes.status, 201); @@ -226,7 +226,7 @@ test('GET /api/transfers returns 200 with readonly token', async () => { test('POST /api/transfers returns 201 with admin token', async () => { const { status, body } = await fetchJson('/api/transfers', { method: 'POST', - headers: { ...authHeader('test-token-admin'), 'Content-Type': 'application/json' }, + headers: { ...authHeader('test-token-admin'), 'Content-Type': 'application/json', 'Idempotency-Key': 'idem-requireScope-228' }, body: JSON.stringify({ senderName: 'Alice', recipientName: 'Bob', amount: 100, from: 'USD', to: 'EUR' }), }); assert.equal(status, 201); @@ -236,7 +236,7 @@ test('POST /api/transfers returns 201 with admin token', async () => { test('POST /api/transfers returns 201 with transfers-scoped token', async () => { const { status, body } = await fetchJson('/api/transfers', { method: 'POST', - headers: { ...authHeader('test-token-transfers'), 'Content-Type': 'application/json' }, + headers: { ...authHeader('test-token-transfers'), 'Content-Type': 'application/json', 'Idempotency-Key': 'idem-requireScope-238' }, body: JSON.stringify({ senderName: 'Alice', recipientName: 'Bob', amount: 100, from: 'USD', to: 'EUR' }), }); assert.equal(status, 201); @@ -291,7 +291,7 @@ test('full transfer lifecycle: create → claim with correct scopes', async () = // Create const createRes = await fetchJson('/api/transfers', { method: 'POST', - headers: { ...authHeader('test-token-admin'), 'Content-Type': 'application/json' }, + headers: { ...authHeader('test-token-admin'), 'Content-Type': 'application/json', 'Idempotency-Key': 'idem-requireScope-293' }, body: JSON.stringify({ senderName: 'Alice', recipientName: 'Bob', amount: 200, from: 'USD', to: 'INR' }), }); assert.equal(createRes.status, 201); @@ -317,7 +317,7 @@ test('full transfer lifecycle: create → claim with correct scopes', async () = test('full transfer lifecycle: create → cancel with correct scopes', async () => { const createRes = await fetchJson('/api/transfers', { method: 'POST', - headers: { ...authHeader('test-token-transfers'), 'Content-Type': 'application/json' }, + headers: { ...authHeader('test-token-transfers'), 'Content-Type': 'application/json', 'Idempotency-Key': 'idem-requireScope-319' }, body: JSON.stringify({ senderName: 'Carlos', recipientName: 'Diaz', amount: 500, from: 'EUR', to: 'MXN' }), }); assert.equal(createRes.status, 201); diff --git a/test/transferIdempotency.test.js b/test/transferIdempotency.test.js new file mode 100755 index 0000000..209051a --- /dev/null +++ b/test/transferIdempotency.test.js @@ -0,0 +1,345 @@ +'use strict'; + +const { test, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); + +const { store, reset } = require('../src/store'); +const transferService = require('../src/services/transferService'); +const idempotencyService = require('../src/services/idempotencyService'); +const stellarService = require('../src/services/stellarService'); +const auditService = require('../src/services/auditService'); +const ApiError = require('../src/utils/ApiError'); + +const ACTOR = 'test-token-admin'; +const OTHER_ACTOR = 'test-token-write'; + +const PAYLOAD = { + senderName: 'Alice', + recipientName: 'Bob', + amount: 100, + from: 'USD', + to: 'EUR', +}; + +/** Build the idempotency context the controller would pass. */ +function ctx(key, payload = PAYLOAD, actor = ACTOR) { + return { actor, key, fingerprint: idempotencyService.fingerprint(payload) }; +} + +/** Count how many times the provider was asked to move money. */ +let providerCalls; +const realSubmitPayment = stellarService.submitPayment; + +beforeEach(() => { + reset(); + providerCalls = 0; + stellarService.submitPayment = (...args) => { + providerCalls += 1; + return realSubmitPayment(...args); + }; +}); + +afterEach(() => { + stellarService.submitPayment = realSubmitPayment; +}); + +/** Audit entries recorded for transfer creation. */ +function creationAudits() { + return auditService.getEntries().filter((e) => e.action === 'transfer.created'); +} + +// ============================================================================ +// The original failure mode +// ============================================================================ + +test('a retry returns the original transfer instead of creating a second one', () => { + const first = transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-retry')); + const second = transferService.createTransfer(PAYLOAD, 'req-2', ctx('k-retry')); + + assert.equal(second.id, first.id); + assert.deepEqual(second, first); + assert.equal(store.transfers.size, 1); +}); + +test('a retry produces exactly one provider command and one audit record', () => { + transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-once')); + transferService.createTransfer(PAYLOAD, 'req-2', ctx('k-once')); + transferService.createTransfer(PAYLOAD, 'req-3', ctx('k-once')); + + // This is the assertion that would have failed before the fix: the provider + // was called on every attempt, so three retries moved money three times. + assert.equal(providerCalls, 1); + assert.equal(creationAudits().length, 1); +}); + +test('a replay does not recompute the quote, so a moved rate cannot change the answer', () => { + const first = transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-rate')); + const rateAtCreation = first.rate; + + const replay = transferService.createTransfer(PAYLOAD, 'req-2', ctx('k-rate')); + + assert.equal(replay.rate, rateAtCreation); + assert.equal(replay.sendAmount, first.sendAmount); + assert.equal(replay.receiveAmount, first.receiveAmount); +}); + +// ============================================================================ +// Conflicting reuse +// ============================================================================ + +test('reusing a key with a different payload fails with 409 and moves no money', () => { + transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-conflict')); + const callsAfterFirst = providerCalls; + + assert.throws( + () => + transferService.createTransfer( + { ...PAYLOAD, amount: 250 }, + 'req-2', + ctx('k-conflict', { ...PAYLOAD, amount: 250 }) + ), + (err) => err instanceof ApiError && err.statusCode === 409 + ); + + assert.equal(providerCalls, callsAfterFirst); + assert.equal(store.transfers.size, 1); +}); + +test('the conflict message names the header, so the client knows what to change', () => { + transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-msg')); + + assert.throws( + () => + transferService.createTransfer( + { ...PAYLOAD, recipientName: 'Carol' }, + 'req-2', + ctx('k-msg', { ...PAYLOAD, recipientName: 'Carol' }) + ), + /Idempotency-Key was already used with a different request payload/ + ); +}); + +// ============================================================================ +// Canonical fingerprint +// ============================================================================ + +test('property order does not make a legitimate retry look like a conflict', () => { + const reordered = { + to: PAYLOAD.to, + amount: PAYLOAD.amount, + senderName: PAYLOAD.senderName, + from: PAYLOAD.from, + recipientName: PAYLOAD.recipientName, + }; + + const first = transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-order')); + const retry = transferService.createTransfer(reordered, 'req-2', ctx('k-order', reordered)); + + assert.equal(retry.id, first.id); + assert.equal(providerCalls, 1); +}); + +test('two different keys with an identical payload are two deliberate transfers', () => { + const a = transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-a')); + const b = transferService.createTransfer(PAYLOAD, 'req-2', ctx('k-b')); + + assert.notEqual(a.id, b.id); + assert.equal(store.transfers.size, 2); + assert.equal(providerCalls, 2); +}); + +// ============================================================================ +// Actor scoping +// ============================================================================ + +test('the same key from a different actor is a separate operation', () => { + const mine = transferService.createTransfer(PAYLOAD, 'req-1', ctx('shared-key')); + const theirs = transferService.createTransfer( + PAYLOAD, + 'req-2', + ctx('shared-key', PAYLOAD, OTHER_ACTOR) + ); + + // Keys are chosen by clients. Without actor scoping one caller could collide + // with another's key and be handed back a transfer that is not theirs. + assert.notEqual(mine.id, theirs.id); + assert.equal(store.transfers.size, 2); +}); + +test('one actor cannot read another actor transfer by guessing the key', () => { + const theirs = transferService.createTransfer( + PAYLOAD, + 'req-1', + ctx('guessable', PAYLOAD, OTHER_ACTOR) + ); + const mine = transferService.createTransfer(PAYLOAD, 'req-2', ctx('guessable')); + + assert.notEqual(mine.id, theirs.id); +}); + +// ============================================================================ +// Concurrency +// ============================================================================ + +test('a second request arriving while the first is in flight is rejected, not duplicated', () => { + // The service is synchronous, so two requests cannot interleave on their own. + // Re-entering from inside the provider call reproduces the exact window the + // original bug lived in: the first operation has started and has not yet + // recorded a result. Driving it this way tests the reservation rather than + // simulating one. + let reentrantError = null; + stellarService.submitPayment = (...args) => { + providerCalls += 1; + if (reentrantError === null) { + try { + transferService.createTransfer(PAYLOAD, 'req-concurrent', ctx('k-race')); + reentrantError = false; + } catch (err) { + reentrantError = err; + } + } + return realSubmitPayment(...args); + }; + + const transfer = transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-race')); + + assert.ok(reentrantError instanceof ApiError, 'the concurrent attempt should have been refused'); + assert.equal(reentrantError.statusCode, 409); + assert.match(reentrantError.message, /still in progress/i); + assert.equal(store.transfers.size, 1); + assert.equal(transfer.id, [...store.transfers.keys()][0]); + assert.equal(providerCalls, 1); +}); + +test('a concurrent attempt with a conflicting payload reports the conflict, not the race', () => { + // Order matters here: reporting "still in progress" for what is really a + // client bug sends them into a retry loop that can never succeed. + let seen = null; + stellarService.submitPayment = (...args) => { + providerCalls += 1; + if (seen === null) { + const other = { ...PAYLOAD, amount: 999 }; + try { + transferService.createTransfer(other, 'req-x', ctx('k-race2', other)); + seen = false; + } catch (err) { + seen = err; + } + } + return realSubmitPayment(...args); + }; + + transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-race2')); + + assert.ok(seen instanceof ApiError); + assert.match(seen.message, /different request payload/i); +}); + +// ============================================================================ +// Provider failure and retry +// ============================================================================ + +test('a provider failure releases the key so the client retry can still succeed', () => { + stellarService.submitPayment = () => { + providerCalls += 1; + throw new Error('stellar horizon timed out'); + }; + + assert.throws( + () => transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-fail')), + /stellar horizon timed out/ + ); + assert.equal(store.transfers.size, 0); + + // Burning the key on failure would be worse than the duplicate it prevents: + // the client retries correctly, with the same key, and can never win. + stellarService.submitPayment = (...args) => { + providerCalls += 1; + return realSubmitPayment(...args); + }; + + const recovered = transferService.createTransfer(PAYLOAD, 'req-2', ctx('k-fail')); + assert.ok(recovered.id); + assert.equal(store.transfers.size, 1); + assert.equal(creationAudits().length, 1); +}); + +test('a failed attempt leaves no reservation behind', () => { + stellarService.submitPayment = () => { + throw new Error('provider down'); + }; + + assert.throws(() => transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-clean'))); + assert.equal(store.idempotency.size, 0); +}); + +test('a completed key survives a later provider outage', () => { + const original = transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-durable')); + + stellarService.submitPayment = () => { + throw new Error('provider down'); + }; + + // The replay must not reach the provider at all, so an outage cannot turn a + // settled transfer into an error for a client that is merely retrying. + const replay = transferService.createTransfer(PAYLOAD, 'req-2', ctx('k-durable')); + assert.equal(replay.id, original.id); +}); + +// ============================================================================ +// Restart +// ============================================================================ + +test('reservations and transfers are cleared together on restart', () => { + transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-restart')); + assert.equal(store.idempotency.size, 1); + assert.equal(store.transfers.size, 1); + + reset(); + + // Records live in the same store as the transfers, so their durability is the + // store's durability. Clearing together is the property that matters: a + // surviving reservation would replay a transfer that no longer exists, which + // is worse than losing both. + assert.equal(store.idempotency.size, 0); + assert.equal(store.transfers.size, 0); + + const afterRestart = transferService.createTransfer(PAYLOAD, 'req-2', ctx('k-restart')); + assert.ok(afterRestart.id); + assert.equal(store.transfers.size, 1); +}); + +// ============================================================================ +// Backwards compatibility of the service entry point +// ============================================================================ + +test('a call with no idempotency context still creates a transfer', () => { + // Idempotency is actor-scoped and internal callers have no actor. The HTTP + // route requires the header, so every request-driven creation is covered. + const transfer = transferService.createTransfer(PAYLOAD, 'req-1'); + assert.ok(transfer.id); + assert.equal(store.transfers.size, 1); + assert.equal(store.idempotency.size, 0); +}); + +// ============================================================================ +// Fingerprint helper +// ============================================================================ + +test('fingerprint is stable across key order and nesting', () => { + const a = idempotencyService.fingerprint({ x: 1, y: { b: 2, a: 3 }, z: [1, 2] }); + const b = idempotencyService.fingerprint({ z: [1, 2], y: { a: 3, b: 2 }, x: 1 }); + assert.equal(a, b); +}); + +test('fingerprint distinguishes array order, which is meaningful', () => { + const a = idempotencyService.fingerprint({ items: [1, 2] }); + const b = idempotencyService.fingerprint({ items: [2, 1] }); + assert.notEqual(a, b); +}); + +test('fingerprint treats undefined and null alike so an omitted field is stable', () => { + const a = idempotencyService.fingerprint({ a: 1, b: undefined }); + const b = idempotencyService.fingerprint({ a: 1, b: null }); + assert.equal(a, b); +}); diff --git a/test/transferIdempotencyHttp.test.js b/test/transferIdempotencyHttp.test.js new file mode 100755 index 0000000..eebaccd --- /dev/null +++ b/test/transferIdempotencyHttp.test.js @@ -0,0 +1,149 @@ +'use strict'; + +const { test, before, after, beforeEach } = require('node:test'); +const assert = require('node:assert/strict'); + +// Set NODE_ENV before requiring the app so config reads the right value at +// require-time, matching the convention in smoke.test.js. +process.env.NODE_ENV = 'test'; + +const createApp = require('../src/app'); +const { store, reset } = require('../src/store'); + +let server; +let baseUrl; + +before(() => { + const app = createApp(); + return new Promise((resolve) => { + server = app.listen(0, () => { + baseUrl = `http://127.0.0.1:${server.address().port}`; + resolve(); + }); + }); +}); + +after(() => { + if (server) { + server.close(); + } +}); + +beforeEach(() => { + reset(); +}); + +const BODY = { + senderName: 'Alice', + recipientName: 'Bob', + amount: 100, + from: 'USD', + to: 'EUR', +}; + +/** + * POST a transfer, optionally with an Idempotency-Key. + * @param {string|null} key + * @param {object} [body] + * @returns {Promise<{status: number, body: object}>} + */ +async function post(key, body = BODY) { + const headers = { + Authorization: 'Bearer test-token-admin', + 'Content-Type': 'application/json', + }; + if (key !== null) { + headers['Idempotency-Key'] = key; + } + const res = await fetch(`${baseUrl}/api/transfers`, { + method: 'POST', + headers, + body: JSON.stringify(body), + }); + return { status: res.status, body: await res.json() }; +} + +test('POST /api/transfers refuses a request with no Idempotency-Key', async () => { + const { status, body } = await post(null); + + // Failing is the only outcome that cannot silently duplicate a transfer: a + // client that omits the header is not opting out of protection, it is + // unaware it needs it. + assert.equal(status, 400); + assert.match(body.error.message, /Idempotency-Key header is required/i); + assert.equal(store.transfers.size, 0); +}); + +test('POST /api/transfers refuses a blank Idempotency-Key', async () => { + const { status } = await post(' '); + assert.equal(status, 400); + assert.equal(store.transfers.size, 0); +}); + +test('POST /api/transfers refuses an oversized Idempotency-Key', async () => { + // Keys are client-supplied and land in a map, so the length has to be bounded + // or the store can be grown without limit by a caller that never retries. + const { status, body } = await post('x'.repeat(256)); + assert.equal(status, 400); + assert.match(body.error.message, /at most 255 characters/i); +}); + +test('a retried POST returns the original transfer with the original status', async () => { + const first = await post('http-retry'); + assert.equal(first.status, 201); + + const second = await post('http-retry'); + + // 201 again, not 200: replaying the stored result means replaying all of it, + // so a successful retry is indistinguishable from the response it stands in + // for. + assert.equal(second.status, 201); + assert.equal(second.body.id, first.body.id); + assert.equal(store.transfers.size, 1); +}); + +test('a retried POST with a changed amount answers 409', async () => { + await post('http-conflict'); + const { status, body } = await post('http-conflict', { ...BODY, amount: 500 }); + + assert.equal(status, 409); + assert.match(body.error.message, /different request payload/i); + assert.equal(store.transfers.size, 1); +}); + +test('a key is trimmed, so surrounding whitespace does not fork the operation', async () => { + const first = await post('padded-key'); + const second = await post(' padded-key '); + + assert.equal(second.body.id, first.body.id); + assert.equal(store.transfers.size, 1); +}); + +test('an amount sent as a string still replays rather than conflicting', async () => { + // "100" and 100 both validate and produce the same transfer, so treating them + // as different requests would reject a client that re-serialized its payload. + const first = await post('http-coerce', BODY); + const second = await post('http-coerce', { ...BODY, amount: '100' }); + + assert.equal(second.status, 201); + assert.equal(second.body.id, first.body.id); + assert.equal(store.transfers.size, 1); +}); + +test('an unrelated extra field does not read as a conflicting retry', async () => { + const first = await post('http-extra', BODY); + const second = await post('http-extra', { ...BODY, clientNote: 'sent from mobile' }); + + assert.equal(second.status, 201); + assert.equal(second.body.id, first.body.id); +}); + +test('validation still runs before the key is reserved', async () => { + const { status } = await post('http-invalid', { ...BODY, amount: -5 }); + assert.equal(status, 400); + + // A rejected payload must not burn the key, otherwise a client that fixes its + // request and retries with the same key would be locked out of it. + const retry = await post('http-invalid', BODY); + assert.equal(retry.status, 201); +});