feat(transfers): make transfer creation idempotent under retries

[leocagli]

Closes #128

The failure mode

createTransfer called the provider before persisting anything:

const quote = quoteService.getQuote(...);
const settlement = stellarService.submitPayment({ ... });  // money moves here
// ...
store.transfers.set(transfer.id, transfer);                // record appears here

A retry that arrived in that window found no record of the first attempt and moved money again. Each attempt also wrote its own transfer.created audit entry, so the duplicate was hard to see afterwards.

The shape of the fix

The (actor, key) pair is reserved before any side effect runs, and the terminal result is stored once the transfer exists:

Step	What it does
begin	Reserve, replay the stored result, or refuse
	provider call and persistence
complete	Store the result so later retries replay it
release	On failure, so a correct retry can still win

Reserving first is the whole point. Any design that writes the record after the provider call has the same window the bug lived in, just narrower.

Design decisions worth reviewing

Scoped by API token. Keys are chosen by clients, so a bare key namespace lets one caller collide with, or probe for, an operation that is not theirs. Two tests cover this.

The fingerprint covers the fields that determine the transfer, not the raw body. An unrelated extra property must not read as a conflicting retry, and amount is normalized because "100" and 100 both validate and produce the same transfer. Property order is canonicalized at every depth, since JSON key order is not semantically meaningful and a client rebuilding a retry from a hash map will not preserve it. Treating any of those as a 409 would reject legitimate retries.

Conflicting reuse is checked before in-flight state. If a key is reused for a different operation, the client gets the same clear answer whether or not the first call has finished. Reporting "still in progress" for what is really a client bug sends them into a retry loop that can never win.

A replay short-circuits before the quote is recomputed. Rates move. Recomputing would hand the client a different transfer under the same key, which is the duplicate this is meant to prevent.

A provider failure releases the reservation. Burning the key would be worse than the duplicate it prevents: the client retries correctly, with the same key, and gets "still in progress" forever even though nothing is in progress.

Compatibility impact

Idempotency-Key is now required on POST /api/transfers. This breaks clients that omit it, and that is deliberate on a money path: a client with no key is not opting out of protection, it is unaware it needs it. Rejecting with 400 is the only outcome that cannot silently duplicate a transfer.

The service entry point keeps the context optional. That is not a loophole, it is what "actor-scoped" implies: an actor exists at the HTTP boundary (req.token) and internal callers have none to scope to. The route supplies the context on every request-driven creation, and a test asserts the route rejects a missing header.

Tests

28 new, 19 at the service level and 9 over HTTP.

The concurrency case is the one I would look at first. The service is synchronous, so two requests cannot interleave on their own; the test re-enters createTransfer from inside the provider call, which reproduces the exact window the bug lived in rather than simulating one:

stellarService.submitPayment = (...args) => {
  try { transferService.createTransfer(PAYLOAD, 'req-concurrent', ctx('k-race')); }
  catch (err) { reentrantError = err; }
  return realSubmitPayment(...args);
};

Coverage: retry replays without duplicate movement, exactly one provider command and one audit record across three attempts, conflicting payload gives 409, concurrent attempt gives 409, conflict beats race in the ordering, provider failure releases the key and the retry succeeds, a completed key survives a later outage, restart clears reservations and transfers together, same key from a different actor stays separate, and the fingerprint canonicalization cases.

The regression assertion for the original bug is providerCalls === 1 after three retries. Against the old code that is 3.

Verification

tests 181
pass  181
fail  0

Suite goes from 153 to 181. test/requireScope.test.js and test/moneyPrecision.test.js were updated to send the header on their POST /api/transfers calls, each with its own key. No assertion was relaxed, nothing was skipped, and a shared key was deliberately avoided since it would have made the second create replay the first and quietly void those tests.

Not done here

Reservation records inherit the store's durability, which for the in-memory Map that backs this demo means a restart clears them. They live in the same store as the transfers on purpose, so the two clear together: a surviving reservation would replay a transfer that no longer exists. Moving to a real datastore makes both durable without touching this logic, which is why I did not add a separate persistence path for it.