Repository navigation
ENG-695: make initiateCashout single-use per offerId - #528
Merged
Merged
Conversation
…ECUTED A cashout offer is about to become single use (ENG-695). Add the typed error a second execution will get, and map it to a stable code the mobile app can branch on, with copy that is safe to show as is: "This offer has already been used. Request a new cashout offer." It extends CacheServiceError beside OfferNotFound, so it joins ApplicationErrors through the existing CacheErrors spread, and the exhaustive switch in error-map.ts maps it through the generic forward-to-client helper the Fygaro and bank-account errors use. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XkM7Bm1cnzugSFi6pGnoWj
executeCashout loaded the offer with a plain GET and never removed it, so every call with the same offerId inside the offer's one-hour TTL reached ValidOffer.execute again. Run against unmodified main, a sequential retry and a concurrent duplicate each drafted and submitted a second ERPNext cashout and called Ibex.payInvoice a second time; the only remaining guard was IBEX refusing an already-paid invoice. Storage.consume now takes the offer out of Redis with consumeCacheKey, one DEL whose removed-count is true for exactly one caller. It runs after revalidation, so a transient validation failure (an ERPNext bank-account lookup, an expired invoice) does not burn the offer, and before execute(), the only step that drafts and pays. The loser of a concurrent race gets OfferAlreadyExecuted and a failed ops event with step "consumeOffer". A Redis error on the DEL refuses the payout (fail closed) and fires the same event with the error name. Before the DEL, consume writes a short-lived tombstone (offers:executed:<id>, same TTL as the offer). Storage.get reads it only when the offer record is missing, so a sequential retry gets OfferAlreadyExecuted instead of OfferNotFound, without reaching validation, ops events or IBEX. The tombstone is best effort and only makes the error specific; the DEL alone guarantees one execution. A consumed offer is never executed again, even if execute() fails before or after paying: the user requests a new offer, and partial failures stay manual-reconcile as before. The offer key and value shape do not change, so offers created before the deploy are consumed the same way. The integration cases turn off cashout.skipPayment (set in the dev config the suite loads) so the IBEX leg is on the path and its call count is evidence. The concurrent case holds both executions inside validation until both have read the offer, so the DEL decides the race. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XkM7Bm1cnzugSFi6pGnoWj
The resolver turned every executeCashout error into a bare
InternalServerError and recorded it as a Critical exception, so mapping
OfferAlreadyExecuted in error-map.ts alone would never reach a client.
Return it as { errors: [{ code: "OFFER_ALREADY_EXECUTED", ... }],
id: null } instead, and keep it out of the Critical exception stream:
a repeated or concurrent execution of one offer is expected, not a
fault. The client can branch on the code and request a new offer.
Every other error still takes the existing catch-all, unchanged; the
new resolver spec pins that so this change cannot widen it by accident.
No schema change: InitiatedCashoutResponse already has errors and a
nullable id, and Error already carries code.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XkM7Bm1cnzugSFi6pGnoWj
…ever arrives The concurrent case holds both executions inside validation until both have read the offer. If a regression stopped one of them earlier (for example, consuming the offer before validation), the other waited on the barrier until the 30 s jest timeout. A 5 s safety release now lets it through, so the test fails on the arrived count (expected 2, received 1) with a clear message. The timer is cleared when both executions settle, and it cannot make the race pass falsely: a caller that arrives only after the release still had to read the offer first. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XkM7Bm1cnzugSFi6pGnoWj
… copy Review fixes for ENG-695 (PR #528). - Storage: move the consumed-offer tombstone from offers:executed:<id> to offer-executed:<id>. initiateCashout passes the client's offerId straight to Storage.get, so the id "executed:<id>" read the tombstone back as a PersistedOffer whose details were 1, and executeCashout threw a TypeError instead of returning a typed error. Storage.get and Storage.consume now also answer any id that is not a UUID with OfferNotFound, before a key is built. - OFFER_ALREADY_EXECUTED copy: the app shows the message as is, and the error mostly follows a retry after a lost response, when the first cashout went through. "Request a new cashout offer" invited a second withdrawal and a second fee. The new copy sends the user to their transactions first and stays true when the first execution failed before any money moved. - Integration: an offer whose execute() fails at IBEX (timeout) is not executed again on retry (one draftCashout, one payInvoice), and an offerId that names a tombstone returns OfferNotFound. Unit: over an in-memory Redis with ioredis-cache's JSON encoding, the forged id gets OfferNotFound and nothing but offer records stays under offers:; non-UUID ids never reach Redis. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XkM7Bm1cnzugSFi6pGnoWj
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
CashoutManager.executeCashoutloaded the offer with a plain RedisGETand never removed, locked or marked it. AnyinitiateCashoutwith the sameofferIdinside the offer's one-hour TTL reachedValidOffer.execute()again.execute()drafts the ERPNext cashout before it pays IBEX, so even if IBEX refused to pay an already-paid invoice, every duplicate would still leave a second ERPNext draft.Confirmed by test on unmodified main (374e5b9) and recorded on ENG-695. One offer executed twice in sequence, and one offer executed twice concurrently, each produced 2
ErpNext.draftCashout, 2Ibex.payInvoiceand 2ErpNext.submitCashoutcalls. Both calls returnedInitiatedCashout.Change
OfferAlreadyExecutedis a newCacheServiceErrorbesideOfferNotFound.error-map.tsmaps it to codeOFFER_ALREADY_EXECUTEDwith the message "This cashout offer was already submitted and can't be reused. Check your transactions before starting a new cashout.", forwarded to the client. The app shows the message as is, and the error mostly follows a retry after a lost response, when the first cashout may have paid, so the copy sends the user to their transactions before a new cashout.Storage.consume(id)takes the offer record out of Redis with the existingconsumeCacheKey: oneDEL,trueonly for the caller whoseDELremoved the key. It is the same gate the Plaid link-token store uses.executeCashoutcalls it after revalidation and strictly beforeexecute():ValidOffer.from, so a transient validation failure (an ERPNext bank-account lookup, an expired invoice) does not burn the offer.execute(), the only step that drafts the ERPNext cashout and pays IBEX.OfferAlreadyExecutedand the ops eventphase: "failed",step: "consumeOffer",error: "OfferAlreadyExecuted".DELrefuses the payout (fail closed) and fires the same event with the error name, for exampleUnknownCacheServiceError.DEL,consumewritesoffer-executed:<id>with the offer's 3600 s TTL, best effort.Storage.getreads it only when the offer record is missing. A later retry then getsOfferAlreadyExecutedinstead ofOfferNotFound, atStorage.get, before validation, ops events or IBEX, and it emits no ops event. TheDELalone guarantees one execution; the tombstone only makes the error specific. The tombstone sits outside theoffers:prefix, so no client-suppliedofferIdcan name it, andgetandconsumeanswer any id that is not a UUID withOfferNotFoundbefore building a key.initiateCashoutreturns{ errors: [{ code: "OFFER_ALREADY_EXECUTED", ... }], id: null }and does not record a Critical exception. Every other error still takes the existing catch-all, unchanged.yarn check:sdlis clean). The offer key and value shape are unchanged, so offers created before the deploy, or by pods not yet rolled, are consumed the same way.For ops: what changes
execute()fails, whether before money moves (adraftCashouterror) or after (IBEX pending or failed, ERPNext submit failed after retry). The user has to request a new offer. Partial failures (paid, but the ERPNext submit failed) stay manual reconciliation, as today.DELerror refuses the payout. Each cashout adds two Redis round trips (tombstoneSET, thenDEL).initiatedfollowed byfailedwithstep: "consumeOffer"in the ops feed. A sequential retry of a used offer is rejected silently; it moves no money.How it was tested
execute-offer.spec: real Redis and Mongo from local docker, with IBEX and ERPNext jest-mocked.cashout.skipPaymentis true in the dev config the suite loads, so these cases turn it off to put the IBEX leg on the path.OfferAlreadyExecuted.draftCashoutandpayInvoiceare each called once.OfferAlreadyExecuted. OnedraftCashout, onepayInvoice. A 5 s safety release makes a regression fail on the arrived count instead of hanging until the jest timeout.payInvoicereturns an IBEX timeout, the retry getsOfferAlreadyExecuted, and there is onedraftCashoutand onepayInvoice.offerIdofexecuted:<id>returnsOfferNotFound.storage/redis-consume.spec(new): a wonDELreturnstrue. A lost one returnsOfferAlreadyExecuted. ADELerror is returned as is. The tombstone is written with the offer TTL before theDEL. A failed tombstone write still consumes and logs.getreturnsOfferAlreadyExecutedon a miss with a tombstone,OfferNotFoundon a miss without one or when the tombstone read fails, and the stored offer on a hit without reading the tombstone.addkeeps theoffers:<id>key and the one-hour TTL. Over an in-memory Redis with ioredis-cache's JSON encoding,get("executed:<id>")after a consume returnsOfferNotFound, and nothing but offer records stays underoffers:. Ids that are not UUIDs never reach Redis.cashout-manager-authz.spec: consume runs before execute. A lost consume or a Redis error stops execute. A validation failure, a wallet on another account, and another account's offer presented with the caller's own wallet all leave the offer unconsumed.ops-events-cashout-manager.spec: a lost consume and a Redis refusal each emit onefailedevent withstep: "consumeOffer"and the error name, and nosucceededevent.initiate-cash-out.spec(new):OfferAlreadyExecutedresolves to{ errors: [OFFER_ALREADY_EXECUTED], id: null }with no Critical exception recorded. Success returns{ errors: [], id }. Other errors keep the generic path.error-map.spec: the code, the copy,forwardToClient, andOfferNotFoundstill mapping toNOT_FOUND.consumeright afterStorage.get(ahead of the ownership checks and validation), 5 unit cases fail (the three that require an unconsumed offer and the two ops-event cases) and 2 integration cases fail (the concurrent case and the validation-failure case). Re-adding the offer whenexecute()fails fails the IBEX-timeout case (the retry pays again). The oldoffers:executed:<id>tombstone fails the forged-id integration case withTypeError: Cannot read properties of undefined (reading 'userAcct'), and each half of the storage fix (the key prefix and the UUID guard) fails its own unit cases when reverted alone.make unit-in-ci(295 suites, 3825 passed, 3 skipped),yarn test:integration(5 suites, 37 passed),yarn tsc-check-noimplicitany,yarn tsc-check,yarn eslint-check,yarn buildandyarn check:sdl(no schema diff),yarn check-yaml,yarn madge-check, prettier 3.6.2 on the changed files, andtypos --config typos.toml. All pass.Follow-ups
cashoutIdonOfferAlreadyExecutedso a client that lost the first response can reconcile. Needs a schema change.OfferNotFoundincluded, into a bareInternalServerErrorinstead of{ errors, id }. It needs its own ticket. The new resolver spec pins today's behaviour, so that fix will update it.OFFER_ALREADY_EXECUTED, send the user to their transactions, then offer a new cashout.DELper key (single instance or sentinel today,src/services/redis/connection.ts). Re-verify if offers ever move to a cluster or a multi-primary setup.scripts/deploy-test.sh flash. Prod apply is manual.🤖 Generated with Claude Code
https://claude.ai/code/session_01XkM7Bm1cnzugSFi6pGnoWj