fix: #1840 release the idempotency key on a definite failure - #2372
fix: #1840 release the idempotency key on a definite failure#2372vaibhav4046 wants to merge 3 commits into
Conversation
About the
|
|
Following up on the environment note with the actual comparison, since asserting "no delta" without showing it is not worth much. Same eight files, run individually with the identical command on this branch and on untouched
Seven of the eight are byte-identical. The eighth misled me at first: a batched run showed 6 failing files on So the delta from this PR is zero, and none of the seven genuinely-failing files import anything it touches. Worth flagging separately, since it is yours rather than mine: on a Windows checkout those seven fail before any change, and |
|
Pushed two cleanup commits. No behaviour change; this is only to make the diff reviewable.
Sorry for the noise in the first push. One thing to know before you look at CI, since I would rather flag it than have you find it.
Same command, same tree, different answers, and the failure is always |
2c63c33 to
16175fd
Compare
|
Rebased cleanly on latest Follow-up for the reconciler path is tracked in #2373 with the architectural questions answered. |
joelorzet
left a comment
joelorzet
left a comment
There was a problem hiding this comment.
The reasoning here is sound and the sequencing behind #2020 was respected. failExecution and completeExecution do adjudicate the question, the routes were discarding that verdict with status === "completed" ? "success" : "failed", and centralising the rule is right because five routes needing the identical mapping means one drifting is indistinguishable from the bug returning.
Putting the rule in an import-free module is a good call for the reason you give: lib/idempotency.ts reaches the database, so route tests mock it wholesale and would otherwise assert against a copy that can never catch a regression in the real rule.
I traced the hazard the #1840 gate warned about, and it is closed everywhere a hash exists. chain-adapter/evm.ts:462 is a deliberate catch-all that turns every post-broadcast failure into OnChainPendingError carrying the hash, including unknown ethers codes, and resolveSponsoredSendError applies the same discipline on the Turnkey path. So once a tx object exists, failed genuinely means definite.
One window is not covered, and the fix is already written elsewhere in this PR. Inline.
| idem, | ||
| NextResponse.json(responseBody, { status: HttpStatus.ACCEPTED }), | ||
| outcome.status === "completed" ? "success" : "failed" | ||
| dispositionForExecutionOutcome(outcome.status) |
There was a problem hiding this comment.
This calls the helper unconditionally, and failExecution answers failed from the mere absence of a hash. The node route recognises exactly this and guards it:
// Only a hash lets failExecution adjudicate anything ...
const disposition = transactionHash ? dispositionForExecutionOutcome(settled.status) : "failed";The four chain-write routes need the same guard, for a narrower reason than the node route's.
The case is a lost response on the send call itself: the node accepts eth_sendRawTransaction and puts the transaction in the mempool, then the reply never arrives. The client throws with no tx object, so no hash is ever assigned. That is not something the error classifiers can fix, since "request never arrived" and "request arrived, reply lost" are indistinguishable to the caller.
#1840's rule is "released when the outcome is definite and nothing landed, held when the outcome is unknown". A lost send reply is unknown, but with no hash this path reads it as definite and releases. Nonces come from getTransactionCount(address, "pending") (nonce-manager.ts:212, transaction-manager.ts:433), so a retry sees the stranded transaction, takes the next nonce, and both land.
Before this change that window held the key, which was the liveness cost #1840 was filed about. This is the one place the change trades it for a possible second broadcast.
Applying the node route's guard here, and at contract-call/route.ts:259, [...slug] and check-and-execute, keeps the intended behaviour and closes it: no hash means nothing was adjudicated, so hold.
There was a problem hiding this comment.
Applied across all four routes (transfer, contract-call, check-and-execute, and [...slug]):
const disposition = result.transactionHash
? dispositionForExecutionOutcome(outcome.status)
: "failed";You are completely right about the lost-send-reply window: without a hash, failExecution answers failed from the absence of a hash alone, which would have released the key and let a retry allocate pending nonce+1 over a transaction sitting in the mempool. Holding the key on "failed" when result.transactionHash is absent closes that window.
Updated tests/unit/execute-protocol-idempotency-disposition.test.ts to assert that an unhashed write failure holds the key ("failed" disposition).
Rebased cleanly on latest staging (2df88cec6), specs/api-coverage.json regenerated with forward slashes, tsgo --noEmit and biome check clean, and all 43 tests passing.
joelorzet
added
the
changes-requested
16175fd to
a3c17bc
Compare
|
Updated with the requested guard from review:
|
An idempotency key is now released when the outcome is definite and nothing is
left in flight, and held when the outcome is unknown.
The platform already computes that distinction. failExecution and
completeExecution return "failed" or "unconfirmed", and the chain decides which:
a conclusive receipt means failed, an unreadable one means the broadcast may
still land. The direct-execution routes discarded the verdict by writing
`outcome.status === "completed" ? "success" : "failed"`, collapsing both into a
single held key. A caller whose transaction was rejected by the staticCall
preflight, before a nonce was even allocated, then replayed that rejection for
the full 24-hour window and could never recover on the same key, while rotating
the key to escape it reopens the double-broadcast hole the key exists to close.
The rule lives in lib/idempotency-disposition.ts, an import-free module so route
tests that mock @/lib/idempotency (which reaches the database) still test the
real rule rather than a mock copy that could drift.
The disposition across all execution routes (transfer, contract-call,
check-and-execute, [...slug], and node) is guarded by transactionHash:
const disposition = result.transactionHash
? dispositionForExecutionOutcome(outcome.status)
: "failed";
Without a transactionHash, failExecution answers "failed" from the mere absence
of a hash. In the lost-send-reply window where eth_sendRawTransaction entered
the mempool but the client timed out before receiving the tx object, releasing
the key would allow a retry to allocate pending nonce+1 and cause both transactions
to land. Requiring result.transactionHash before adjudicating "failed" as "release"
ensures unhashed failures hold the key ("failed") for the 24h window.
Exceptions handled:
- completeExecution reported a hash it could not check against a chain
(chainId undefined) as "failed". It now returns "unconfirmed".
- On the node route, unhashed step failures hold the key because an arbitrary
node step may have performed side effects before failing.
Documentation updated in docs/api/direct-execution.md and specs/api-coverage.json
regenerated. Comprehensive unit and route test coverage added.
a3c17bc to
468be65
Compare
|
The lost-send window is closed. All four chain-write routes now carry the node route's guard - Three things the increment surfaces that are worth settling before it lands. The docs now contradict the code, and a caller following them hits the original bug. That is also the honest cost of the fix, and the PR does not name it. The increment inverted the test that encoded #1840's reported case - The release predicate is Two qualifications I want to be accurate about rather than overstate. It is currently latent - There is a live producer of hash-without- The downstream consequence is new and not yours to fix here, but it should be filed: the row is now Two smaller ones. Nothing above changes my read that the centralisation is the right call. |
…h the code Addresses the three points raised in review on the previous increment. The release predicate was reverted or safe_inner_failure, not reverted alone verify-receipt puts success, reverted and safe_inner_failure in CONCLUSIVE_STATUSES, so failExecution derived "failed" from a safe_inner_failure receipt and idempotency-disposition maps that to release. That branch is only reachable below the receipt.status === 0 early return, so the outer execTransaction mined: the Safe's nonce was consumed and the owner signatures for it were spent. Releasing there lets a retry spend a second nonce and a second signature set. It is latent today and the fix is still worth landing. Transactions this codebase builds pass safeTxGas=0, baseGas=0, gasPrice=0, so Safe's own require reverts the outer transaction to status 0 and the plain status check catches it first. A path that submits a Safe transaction it did not construct, such as executing one queued in the Safe UI where the proposer sets safeTxGas, reaches it immediately. The precise claim: the inner call reverted, so the intended work did not happen. What took effect is the transaction itself, its nonce and gas, and with non-zero gasPrice or safeTxGas a refund. That is not a double-application, but it does falsify the "nothing landed" contract that release rests on. The docs contradicted the code, and a caller following them hit the bug docs/api/direct-execution.md still said a rejection caught before the transaction was ever submitted releases the key. That has been false on all four chain-write routes since the no-hash guard landed: a pre-submission rejection has no hash, takes the "failed" branch, and is held for 24 hours. A caller who read that paragraph and re-sent under the same key got a replay of the rejection, which is the symptom the paragraph was written to describe. Rewritten to tie release to what actually causes it: a hash that verified to a conclusive on-chain failure, and only that. It now names both consequences a caller has to plan for, the held pre-submission rejection and the held Safe inner failure. Which of the two behaviours this ships Stated plainly rather than left to be inferred. This ships the conservative hold: with no hash there is nothing to adjudicate, "never sent" and "sent, reply lost" are indistinguishable at that point, and a retry that guesses wrong takes the next pending nonce alongside a transaction already in the mempool. The reported case in the original issue, a staticCall rejection, is therefore held again as it was before this work. Recovering that liveness needs the write core to expose whether a send was ever issued, which is knowable there and is not on the result type today. errorClass is a fault domain rather than a broadcast fact and cannot stand in for it. That is a change to the write core's contract and its own increment, not a rider on this one. lib/idempotency.ts no longer claims a call that reached the broadcast path is never released, and failExecution now records that its hash-and-chainId pairing rests on convention across the routes and plugin steps rather than on the type. tsgo --noEmit clean, biome clean on both changed files, and the disposition suite passes 8/8 both with and without this change.
|
All three settled, plus the two smaller ones. Pushed as The release predicate. You are right, and it is const spentASafeNonce = receipts.some(
(receipt) => receipt.receiptStatus === "safe_inner_failure"
);
const status =
receipts.length > 0 &&
(isInconclusive(receipts) || landedSuccessfully || spentASafeNonce)
? "unconfirmed"
: "failed";I took your qualification rather than the stronger claim I would have written on my own. The comment says the inner call reverted so the intended work did not happen, and what took effect is the transaction - nonce, gas, and with non-zero The docs. Which of the two I intend to ship, said out loud. The conservative hold. With no hash there is nothing to adjudicate, "never sent" and "sent, reply lost" are indistinguishable at that point, and a retry that guesses wrong takes the next pending nonce alongside a transaction already in the mempool. So #1840's reported case, a staticCall rejection, is held again as it was before this work, and the PR should not pretend otherwise. That is the honest cost and it is now in the commit message rather than left to be inferred. Recovering that liveness is the change you described: the write core knows whether a send was ever issued, every no-hash failure return sits above the broadcast, and nothing on The two smaller ones. Reachability in the PR description. Corrected, per your note on #2373. The Filed separately: #2432 for the downstream consequence - that row passes the reconciler's filter, Still owed: the three uncovered routes. You are right that the guard is copy-pasted four times with one copy pinned. Disposition tests for
|
The guard was copy-pasted four times with one copy pinned, and the module's own comment says one of them drifting is indistinguishable from the bug coming back. Only [...slug] had a disposition test. Nine tests added for transfer, contract-call and check-and-execute, in the existing file rather than a new one, because the point is that one rule is pinned in one place. Each route gets three cases: a write that fails with no transaction hash holds the key, a write whose hash verified to a conclusive on-chain failure releases it, and a successful write finalises. Each was verified to fail when the guard it pins is removed, with two mutations per route because the hash check and the mapping pin different tests. Dropping the hash check turns the hold case from "failed" to "release"; hardcoding "failed" turns the other two red. The four routes were confirmed byte-identical to their prior state after every mutation. Harness notes, since the additions are not free: the read-contract mock became controllable because check-and-execute needs its condition read to resolve, and @/lib/execute/simulate plus transfer-token-core are stubbed. Neither is reachable on a broadcasting write - they are stubbed because their module graph pushed the in-test dynamic import past the 10s timeout. With them stubbed the whole file runs in about 9s. Coverage is deliberately not four-way on everything: the unconfirmed verdict stays pinned once, on [...slug], since all four call the same dispositionForExecutionOutcome. What is pinned four times is the part that was copy-pasted four times. 17 passed. tsgo --noEmit clean, biome clean on the test file.
|
Second push, Nine added for Each verified to fail when the guard it pins is removed. Two mutations per route, because the hash check and the mapping pin different tests - dropping the hash check turns the hold case from All four routes confirmed byte-identical to their prior state after every mutation. Two things worth flagging rather than leaving to be discovered. The additions are not free. The read-contract mock had to become controllable because check-and-execute needs its condition read to resolve, and Coverage is deliberately not four-way on everything. The
That is everything from your review. The write-core change - exposing whether a send was ever issued, so a pre-submission rejection can release again - is the one thing I have left out on purpose, as its own increment rather than a rider on this. Say the word and I will open it. |
3 participants
Issue
Closes #1840
The gate stated on that issue has cleared. On 2026-08-13 the decision was recorded as:
#2020 closed on 2026-08-28 via #2162, with #2177 following via #2192 on 08-31. This is the change that was sequenced behind it, in the narrow form that was decided.
What this changes
failExecutionandcompleteExecutionalready adjudicate the only question that matters: the chain decides, and a receipt that cannot be read yieldsunconfirmedrather thanfailed. The direct-execution routes discarded that verdict:Both
failedandunconfirmedlanded on the held disposition. A transaction thestaticCallpreflight rejected before a nonce was allocated then replayed that rejection for the full 24-hour window, and the same key could never recover.completedsuccessfailedreleaseunconfirmedfailed(held)Applied to all five
/api/execute/*routes:contract-call,transfer,check-and-execute,[...slug],node. One route fixed and four siblings left with the same collapse is the failure mode ISSUES.md calls out, and a rule written out five times is one refactor away from this returning in one of them, so it lives inlib/idempotency-disposition.ts.Three things a reader would not predict from the title:
completeExecutionreported an unverifiable hash as failed. Withresult.chainIdundefined there is a transaction hash and no chain to check it against, and the old code called that"failed". That is the case the!allVerifiedbranch beside it already argues about. It now returns"unconfirmed". Without this, releasing onfailedwould free the key for a retry that re-broadcasts a transaction which may already have landed, so the change would have introduced the bug it exists to prevent.On
node, "no hash" is not evidence that nothing happened. Unlike the chain-write routes, an arbitrary step there may have sent a message or delivered a webhook before reporting failure, andfailExecutionanswers"failed"from the mere absence of a hash. That route releases only when a hash existed to adjudicate. The two paths that callfailExecutionwith no receipt at all keep holding the key and say why.The documentation was made false and is fixed in the same PR.
docs/api/direct-execution.mdtold callers to rotate after a definite result because "A stored failure is replayable for 24 hours". It now separates a definite failure, which releases, from an unreadable receipt, which is held and replays, and points atGET /api/execute/{executionId}/statusinstead of rotation for the second case. Nothing else in the repository repeats the old claim.specs/api-coverage.jsonis regenerated bypnpm check:api-docs, which is idempotent on a second run.Scope
One change. Every part is the same rule: a definite outcome frees the key, an unknown one holds it. The
completeExecutionfix is not separable, because releasing onfailedwhilefailedcan still mean "hash we could not check" is the double-broadcast bug the issue thread explicitly refuses to ship. Thenodegate and the docs edit are the same rule stated for one more route and for callers.Deliberately not included, though it is the natural next step: the reconciler is the only thing that can turn an unknown outcome into a definite one after the request has gone, and it never touches idempotency, so a held key stays held even once the chain answers. That is independently shippable and correct with this PR reverted, so per CONTRIBUTING.md it is a separate issue. I have it written and will file it rather than fold it in.
How it was verified
pnpm type-checkclean.biome checkclean on all ten changed files.pnpm check:api-docsgreen, twice.Tests, and what they would catch:
tests/unit/idempotency.test.tscovers the rule directly, including that the three outcomes map to three distinct dispositions.tests/unit/execute-protocol-idempotency-disposition.test.tscovers both directions of the pair that constrains this issue: a conclusive revert releases, an unreadable receipt holds, and the reported preflight rejection releases so the same key executes again instead of replaying.One existing expectation moved from
"failed"to"release". The test named "finalizes as failed when the write reverts after broadcast" encoded the collapse, so it is renamed and now asserts the released key. That is the behaviour change the issue asked for, not a test bent to fit.Checked that the tests fail without the fix, by mutation rather than by assertion. Collapsing
failedback onto the held disposition:The first run exposed a real weakness. The route test has to mock
@/lib/idempotencybecause that module reaches the database, and it mirrored the rule inside its own mock factory, so it could never fail if the real rule regressed. The rule therefore moved tolib/idempotency-disposition.ts, a module with no imports;lib/idempotency.tsre-exports it so no route import changes; and the route test now pulls the real function throughvi.importActual. The same mutation now kills tests in both files.Screenshots
Nothing renders.
stagingEnvironment note, not a claim about this change: my checkout is Windows with
core.autocrlf=true, so a repo-widepnpm checkreports CRLF errors on all 2191 files and a fulltests/unitrun has pre-existing failures. I normalised only the files I touched, and compared the suite against cleanstagingper-file rather than assuming: seven files fail identically before and after, andworkflows-execute-alias.test.tsis a 10s timeout flake that fails 4 of 5 runs on untouchedstaging. Detail in the comment below.