Skip to content

adr: 0020 title: "Slippage measured against the published price; idempotency keys released on rejection" status: accepted implementation-status: implemented date: 2026-06-03 implemented-date: 2026-06-03 implemented-in: - ftl-backend bug/trade-price-moved-500 - ftl-frontend bug/trade-price-moved-retry deciders: [@amalkrsihna] affects-specs: [amm-pricing, cfd-pricing-and-wallet] affects-code: - ftl-backend/internal/redis/lua/position_open.lua - ftl-backend/internal/redis/lua/position_close.lua - ftl-backend/internal/positions/service.go - ftl-backend/internal/positions/handlers.go - ftl-backend/internal/config/config.go - ftl-frontend/src/components/CFDTradeForm.tsx supersedes: null superseded-by: null

ADR-0020: Slippage measured against the published price; idempotency keys released on rejection

Context

During the live U18 match on the dev02 / staging stack, opening (or closing) a CFD position frequently returned 409 PRICE_MOVED, and pressing "confirm again" then returned HTTP 500 "internal error". Two independent defects combined:

  1. Spurious PRICE_MOVED. The slippage guard in position_open.lua / position_close.lua compared the user's click price against last_noisy_price — the generator's internal tick value — with a 0.5% tolerance (DEFAULT_SLIPPAGE = 0.005, TRADE_SLIPPAGE_TOLERANCE_PCT = 0.005). The synthetic noise / pattern engine (ADR-0018) moves last_noisy_price between WS publishes, and the delta-skip filter means many of those internal moves are never broadcast. So the server rejected the trade against a price the user was never shown, and a 0.5% band was far tighter than the per-tick noise amplitude.

  2. The 500 (idempotency-key burn). On a PRICE_MOVED rejection the Lua had already claimed the idempotency key (SET idem:<user>:<crid> NX) but returned without writing the result cache. A retry with the same clientRequestId found the key set and no cached result, so it returned {status:"duplicate", openPrice:0}. service.Open then hit its defence-in-depth if result.OpenPrice <= 0 { return ErrCorruptPositionState } guard (ADR-0016), and openErrorStatus had no case for that error → the default 500 "internal error". A harmless re-quote looked like a server crash.

The idempotency contract (ADR-0002) exists to make a completed trade safe to retry — a network drop on the response must not double-open. It was never meant to persist across a rejected trade that changed no durable state. The bug #135 fix had already moved pre-claim instrument checks ahead of the claim for exactly this reason; the post-claim rejections (slippage, SL/TP, margin, wallet) were never given the same treatment.

Decision

1. Slippage is measured against the last published price. Both Lua scripts now read last_published_price (the value actually broadcast over the WS, i.e. what the user saw) and fall back to last_noisy_price → server_price when it is unset (no live ticks yet). position_open.lua folds these two fields into the existing instrument HMGET so the check adds no extra round-trip.

2. The default tolerance widens 0.5% → 2.5% (DEFAULT_SLIPPAGE in both Lua, TRADE_SLIPPAGE_TOLERANCE_PCT default in config.go), env-tunable. 2.5% comfortably covers a one-tick lag between the published price and the click while still protecting the trade-integrity contract (wallet debit = shown price).

3. A rejection never burns the idempotency key. Both scripts add a reject(resp) helper that DELs the freshly-claimed idem key before returning. It is applied to every rejection that occurs before the first durable mutation; all such rejects were audited to precede the first HSET. The idem key now persists only for a successful trade (via cache_and_return). A retry after any rejection therefore re-evaluates cleanly instead of replaying an empty duplicate marker. The cached-result short-circuit for a genuinely successful replay is unchanged and still runs first, so a completed trade's retry still returns its cached fill (it is never re-evaluated for slippage).

4. Defence in depth on the Go side. service.Open no longer classifies a replayed/duplicate openPrice == 0 as corrupt — it returns the new ErrDuplicateInFlight (HTTP 409, retryable). openErrorStatus maps both ErrDuplicateInFlight and ErrCorruptPositionState to 409, and the Lua invalid stop_loss/invalid take_profit rejects to 422 (they previously fell through to 500). corrupt_position_direction now maps to ErrCorruptPositionState.

5. Frontend mints a fresh key on re-quote. CFDTradeForm resets clientRequestIdRef on PRICE_MOVED so the "confirm again" is a new intent at the new price. With (3) this is belt-and-suspenders, but it keeps the contract explicit.

6. A committed close is never failed by a sibling. Adversarial review found that position_close.lua's post-close equity-snapshot loop hard-failed with instrument_price_missing if another of the user's positions had an evicted instrument hash — returning an error after the close committed, so the retry replayed a zero-price ledger UPDATE. The loop is now best-effort: it skips a sibling whose price is missing rather than abort an already-committed close (the next portfolio fetch re-hydrates the snapshot).

Consequences

  • Positive: a re-quote after PRICE_MOVED can no longer surface as a 500; the retry either fills or returns a clean, typed 4xx.
  • Positive: slippage rejections now reflect the price the user actually saw, and at 2.5% are rare during normal live-match noise — without weakening the wallet-debit-equals-shown-price guarantee on accepted trades.
  • Positive: idempotency now means exactly "a completed side-effectful trade is replay-safe"; rejected no-ops no longer consume a request id.
  • Positive: a committed close is durable regardless of unrelated instrument eviction — closes the zero-price ledger-UPDATE corruption path.
  • Neutral: a wider tolerance means a user can fill up to 2.5% away from the shown price. Acceptable: the fill is still at the user-agreed clientPrice (trade integrity is unchanged), and 2.5% is the maximum divergence tolerated, not the typical fill.
  • Risk: if last_published_price is stale (e.g. an instrument that stopped ticking) the check references an older price. The fallback chain and the live ticker keep this bounded; a genuinely stale instrument is frozen anyway.

Alternatives considered

Alternative A: Keep comparing against last_noisy_price, just widen tolerance

Raise the tolerance enough that internal noise never trips it.

Rejected. The user never saw last_noisy_price; comparing against it is wrong in principle no matter the band. A band wide enough to absorb all internal noise would also stop protecting the trade-integrity contract. Comparing against the published price fixes the cause, not the symptom.

Alternative B: Don't claim the idempotency key until after the slippage check

Move the claim below the slippage/validation block so a rejection never holds it.

Rejected. The cached-result short-circuit MUST run first so a retry of a successful trade returns the cached fill without re-evaluating slippage (which could falsely reject a trade that already debited the wallet). Releasing the key on rejection (reject()) achieves the same "rejections don't burn the key" outcome while preserving the success-replay guarantee.

Alternative C: Map the duplicate-zero result to a generic retry without the Lua change

Only fix the Go side: treat any openPrice == 0 replay as a soft 409.

Rejected as insufficient. That hides the burned key but leaves it set, so the next legitimate attempt with the same id would still be deduped against a trade that never happened. Releasing the key is the correct primitive; the Go mapping is the backstop.

Verification

  • internal/positions miniredis tests run the real Lua: TestOpenRejectReleasesIdemKey and TestCloseRejectReleasesIdemKey assert a stale-price trade returns PRICE_MOVED, references last_published_price, and releases the idem key; the open test further asserts a same-id retry within tolerance opens cleanly. TestCloseSucceedsWhenSiblingInstrumentEvicted asserts a committed close survives an evicted sibling instrument.
  • TestOpenErrorStatus_RetryablesAre4xx and the extended luaErrorToGoErr mapping tests lock the 409/422 responses (never the generic 500).
  • go test ./... green; golangci-lint run 0 issues.