A held idempotency key is never released once the reconciler proves the outcome was a definite failure

[vaibhav4046]

What happens

A held idempotency record is never revisited once the outcome it was waiting on becomes definite.

The route-level rule releases a key when the outcome is definite at request time and holds it when the outcome is unknown. An unreadable receipt is not definite then, so the record is finalized as held. The reconciler is the only thing that can later turn that unknown into a definite answer: it re-reads the receipt and settles the row to completed or failed.

It never touches idempotency. lib/execute/reconcile-executions.ts and app/api/cron/execution-reconciler/route.ts contain zero references to it. The only release path is release() in lib/idempotency.ts, called from route handlers at request time.

So when the reconciler settles a row to failed on a conclusive receipt, nothing landed, the outcome has become definite, and the key should be reusable on exactly the reasoning #1840 applies synchronously. Instead the record keeps replaying the stale unconfirmed body for the full COMPLETED_TTL_MS window.

Why it is narrow

Severity is bounded and I do not want to oversell it. The held body carries executionId and idempotentReplay: true, so a caller can poll GET /api/execute/{executionId}/status and is not blind. The two 24-hour clocks, COMPLETED_TTL_MS and DROPPED_AFTER_MS, roughly coincide, so the window closes on its own. It is a liveness gap, not a correctness or double-spend one.

The part that needs a decision, not code

Only a conclusive receipt should release. The reconciler reaches failed by three paths and the other two are not chain answers:

• chain could not be resolved means the receipt was never read.
• the dropped-after-24h path is a timeout inference about a transaction that could still be sitting in a mempool.

Releasing on either lets a retry broadcast a second transaction for work whose fate is genuinely unknown, which is the failure the key exists to prevent. A verified success must also keep its key, since a retry must not repeat an action that already took effect.

Two smaller things follow from it. recordIdempotentResponse writes resourceId: null on the held path, which discards the only link from an execution back to its record, so that has to be kept. And a release must be scoped to the owning organization and must never touch a processing row, which belongs to an in-flight request holding a fencing token.

Related

Follows #1840 and #2020. It is deliberately not part of #2372: that PR ships and is correct with this absent, so per CONTRIBUTING.md they are separate.

I have this written and tested already (a standalone release scoped to held rows, wired to the reconciler's conclusive branch only, plus a keysReleased counter on the run summary and seven tests covering which verdicts release and which do not). Happy to open the PR once this is triaged, or to leave it if you would rather the reconciler stayed out of idempotency entirely.

[subheeksh5599]

Verified the mechanism against staging (origin/staging @ b3ad967) - the report is accurate, and I want to record the exact handoff before proposing a shape.

Confirming each claim

The key is held at request time when the receipt is unreadable. app/api/execute/[...slug]/route.ts:343-346 finalizes any broadcast-path outcome that is not completed as "failed" (never released). recordIdempotentResponse stores the response body verbatim on that path (lib/idempotency.ts:271-278), so the stored body is the poll-only { status: "unconfirmed", executionId, ... } envelope the route returned (route.ts:321-339).

A retry then replays that stale body for the full 24h. lib/idempotency.ts:206-212 replays both completed and failed records ("a failed record means the prior attempt reached the broadcast path, so re-running could double-spend"). COMPLETED_TTL_MS is 24h (:17). So the caller who retries with the same key after the reconciler has already settled the row gets the stale unconfirmed body until the TTL lapses - the record never transitions when the outcome becomes definite.

The reconciler never touches idempotency. Zero references in lib/execute/reconcile-executions.ts and app/api/cron/execution-reconciler/route.ts (the cron calls reconcileUnconfirmedExecutions at route.ts:35 and nothing else). The only release path is release() from request-time route handlers.

resourceId: null on the failed path is real and is the missing link. lib/idempotency.ts:275 hardcodes resourceId: null in the failed finalize even though the response body carried an executionId - the success path extracts it from the body (:280-283) but the failed path discards it. So a held record has no pointer back to the execution that could resolve it.

The reconciler reaches failed by three paths and only one is a chain answer (lib/execute/reconcile-executions.ts):

1. :171-176 - chain could not be resolved (no receipt read)
2. :190-201 - conclusive revert / safe_inner_failure (results.every(...)) - genuine chain answer, nothing landed
3. :205-212 - dropped after DROPPED_AFTER_MS = 24h (:37) - mempool timeout inference

Proposed shape

Scope: release the held idempotency record only when the reconciler proves a conclusive failure (path 2) - the receipt was read, the chain answered reverted/safe_inner_failure, and nothing can land. Path 1 (never read) and path 3 (may still be in a mempool) keep the key held, since releasing on either would let a retry broadcast a second transaction for work whose fate is unknown - the exact failure the key exists to prevent. A settled completed also keeps its key (a retry must not repeat an action that already took effect).

The link problem and the release are the same change: keep resourceId (the executionId) on the failed finalize so the reconciler can find the record by execution, then release it from the reconciler's conclusive-failure settle path - scoped to the owning organization and never touching a processing row (an in-flight request holds the fencing token).

What I have not settled and would like direction on before writing code:

• Where the release belongs - a function the reconciler calls after settle(), versus teaching settle() itself to return the execution for a follow-up release. The reconciler currently batches (reconcileUnconfirmedExecutions) and I want the idempotency write to reuse the same DB transaction shape rather than adding a second write per row.
• Whether the record for a conclusive failure should be released (dropped, so the next identical request broadcasts fresh) or finalized as a replayable failed with the definitive revert error body. Releasing matches the execute_contract_call: a reused idempotency_key replays a cached failure, so a retry can never recover #1840 reasoning for a definite failure at request time; finalizing keeps a same-key retry from re-broadcasting even after a revert, which is the conservative reading of :204-206. I lean release (the receipt proves nothing landed, so re-broadcast is safe), but this is the decision the issue flags and I would rather build to the maintainer call.

Happy to take this on. If the shape above is right, I will build to it.

[vaibhav4046]

Spot on with the handoff — that confirms the exact mechanism found during the #1840 audit.

On the two design questions:

1. Where the release belongs:
   A focused helper (releaseKeyForConclusiveFailure) called immediately after settle() succeeds (if (settled) { ... }).

   • Gating on settle returning true (updated.length > 0) is critical: only the run that actually won the atomic transition race owns the right to release the idempotency key. A concurrent reconciler that lost the race does not touch the key.
   • The prerequisite is keeping resourceId (executionId) on the held finalize in recordIdempotentResponse (lib/idempotency.ts). Currently it hardcodes resourceId: null on settle === "failed", which strands the record. Keeping resourceId allows an indexed delete:
     DELETE FROM idempotency_records WHERE organization_id = $1 AND resource_id = $2 AND status = 'failed'.
   • Wrapping the release in try/catch with logSystemWarn ensures that even if the idempotency delete has a transient DB issue, the already-settled execution state is not affected.

2. Release vs. replayable failed:
   Release (drop).

   • A conclusive on-chain revert (reverted or safe_inner_failure) proves beyond ambiguity that nothing took effect. Releasing the key allows the caller to re-broadcast fresh on the same logical intent, matching the exact reasoning applied at request-time in execute_contract_call: a reused idempotency_key replays a cached failure, so a retry can never recover #1840.
   • Finalizing as a replayable failure would trap callers in a 24-hour dead-end when their original transaction merely failed due to transient state (e.g. slippage, temporary price move, gas spike) that could succeed on retry.
   • As you noted, Path 1 ("chain could not be resolved") and Path 3 (dropped after 24h mempool timeout) must strictly keep the key held, because neither is a conclusive on-chain receipt and work could still land.

I've rebased PR #2372 on latest staging (eb239c6) so the request-time half is clean, verified, and fully mergeable. I also have the unit tests and implementation for this reconciler half written and verified against the test suite if you want me to put up a branch/PR, or feel free to take the shape above and run with it!

[suisuss]

Confirmed, and every mechanical claim holds. lib/execute/reconcile-executions.ts contains zero references to idempotency - I grepped the file rather than trusting the read - and release() (lib/idempotency.ts:97-99) has one call site, :260, reached only from recordIdempotentResponse, which is only ever called by route handlers. recordIdempotentResponse does write resourceId: null on the held path. The two clocks are not merely close: COMPLETED_TTL_MS (lib/idempotency.ts:17) and DROPPED_AFTER_MS (reconcile-executions.ts:37) are both exactly 24h.

One thing before anything else, because you say the code is already written and this is in the part you have written. Your release predicate reuses the existing conclusive check, which is result.status === "reverted" || result.status === "safe_inner_failure" (reconcile-executions.ts:190-193). Those two are not the same kind of answer. In verifySingleReceipt, receipt.status === 0 returns early as "reverted" (lib/web3/verify-receipt.ts:268-278); safe_inner_failure is only reachable below that check (:280-290), which means receipt.status === 1 - the outer execTransaction mined successfully. The Safe's nonce was consumed and its signatures were spent; only the inner call failed.

Your own rule from #1840 is "definite and nothing landed". safe_inner_failure is definite and something landed. Releasing on it hands a retry the right to re-broadcast work whose outer transaction already took effect - which is the exact collapse of two different outcomes into one bucket that this issue is about, one level down. The predicate has to be reverted alone.

Two prerequisites you did not name, both structural.

There is no index on resource_id. idempotency_records carries uq_idempotency_org_scope_key and idx_idempotency_expires_at and nothing else (lib/db/schema-extensions.ts:757-762). A reconciler-side lookup by resourceId is a full scan per settled row against a table holding 24h of idempotent traffic from every route, and scope cannot narrow it because scopes are built per request (execute:${actionType}, mcp-call:${id}:${protocol}:${addr}). That is a migration, not an optimisation.

And resourceId is ambiguous: pickString(record.executionId) ?? pickString(record.id) ?? pickString(record.workflowId) (lib/idempotency.ts:281-285), first match wins, so it can hold a workflow id. A predicate keyed on resource_id alone can match a record from a different route family and needs a scope or kind constraint.

You are right about the fencing-token hazard and it is sharper than you put it: reclaim reuses the same row id (lib/idempotency.ts:171-200), so an unfenced delete could remove a newer processing row belonging to a different in-flight request. What saves it is that reclaim sets status: 'processing' and resourceId: null, so a predicate of status = 'failed' AND resource_id = <executionId> is self-fencing - worth writing down as a requirement rather than leaving implicit.

On "conclusive revert and finalized": there is no finality primitive on the EVM path to build that from. lookupReceipt accepts whatever getTransactionReceipt returns at any depth and nothing compares blockNumber against a finalized tag; the only confirmation-depth machinery in the repo is Solana-specific. So that caveat is new machinery, not a predicate tweak, and it should be priced before it is promised.

Relevant to #2372, which is yours. #2372 changes completeExecution to return unconfirmed where it returned failed when a hash exists with no chainId. That produces a row with a non-null hash and receipts = [], which now passes the reconciler's eligibility filter, and resolveChainId falls back to bare Number(execution.network) rather than getChainIdFromNetwork - and network can hold a chain name, since KEEP-490 keeps that as a supported alias. Number("base") is NaN, which lands on the chain-could-not-be-resolved branch: a terminal failed stamped without ever reading a receipt. I could not identify a live producer of hash-without-chainId, so treat the reachability as UNSURE - but the asymmetry is real either way, and it is worth checking before that PR merges rather than after.

What is not settled, and it is the question you asked rather than one I am adding. Whether the reconciler should hold a reference to idempotency at all. The alternative is that it never deletes: on settling a row it refreshes responseBody and responseStatus through the same finalize fence, so a replay returns the true outcome instead of a stale unconfirmed. That cures the stale-news complaint on every path including the ones where the rule says hold, and it has no deletion surface and therefore no double-broadcast risk at all. It does not deliver liveness for the revert case, which genuinely needs the key gone.

I'm taking that to the core team - release against refresh, and whether the reconciler owns idempotency knowledge or the coupling goes the other way. Nothing here is blocked on you, and please do not rework the branch until I come back, beyond the predicate fix above, which holds under either answer.

Your severity framing is right and I want to record it: the held body carries executionId and idempotentReplay: true, GET /api/execute/{executionId}/status is org-scoped and returns the settled status, and the MCP tool descriptions already tell agents to poll it and never re-send on unconfirmed. This is a liveness gap for a client that polls, not a correctness or double-spend one.

[vaibhav4046]

Spot on across the board — especially on safe_inner_failure. That distinction is surgical:

1. The Release Predicate: reverted alone

You are 100% right. In verifySingleReceipt, safe_inner_failure is only reached when receipt.status === 1 (the outer execTransaction mined successfully on-chain). The Safe’s nonce was consumed and its signatures were spent; only the internal call failed.

Releasing the idempotency key on safe_inner_failure would allow an agent or retry to re-submit with an already-spent Safe nonce or re-broadcast against an envelope that already took effect. That directly violates the rule from #1840: "definite and nothing landed". For safe_inner_failure, something did land. The predicate must strictly be reverted alone (result.status === "reverted" where receipt.status === 0).

2. Structural Prerequisites

Completely agree on all four:

• resource_id Index: A reconciler-side lookup by resource_id without an index would turn into a full sequential scan across 24h of all idempotency traffic. Adding idx_idempotency_org_resource_id via a schema migration is a prerequisite before any production query by resource_id.
• resourceId Ambiguity: Because resourceId can hold an executionId, id, or workflowId, any lookup must be qualified by organization_id and route scope (e.g. scope LIKE 'execute:%').
• Self-Fencing Token: Good catch on the reclaim lifecycle — since reclaim resets status: 'processing' and resourceId: null, the compound predicate status = 'failed' AND resource_id = <executionId> is naturally self-fencing against races with a newer in-flight request.
• EVM Finality: Acknowledged. With lookupReceipt returning whatever getTransactionReceipt returns regardless of block depth, promising finality on EVM is new confirmation-depth machinery (like Solana's) and should not be conflated with receipt presence.

3. Asymmetry & Reachability in #2372

To clarify the intent behind returning unconfirmed on completeExecution when chainId is missing:

• In fix: #1840 release the idempotency key on a definite failure #2372, status === "failed" maps to "release". If completeExecution had continued returning "failed" when result.chainId === undefined, then a call with a transactionHash would have released the key, opening the double-broadcast hole if the transaction had actually landed. Returning "unconfirmed" maps to held, preserving the core invariant: never release on an unknown broadcast.
• On the reconciler side, you're right: if such a row reached the reconciler and had network: "base", resolveChainId's fallback to Number(execution.network) yields NaN, landing on chain-could-not-be-resolved and failing the row without an RPC check. Crucially, however, even in that worst-case fallback, the reconciler stamps the execution row failed while the idempotency record remains held for 24h (since the reconciler never touches idempotency). The double-broadcast safety invariant holds.
• And as you verified, there is currently zero live producers of hash-without-chainId across the write routes (transferFundsCore, transferTokenCore, and writeContractCore all strictly return numeric chainId alongside transactionHash).
• If you'd like symmetry closed in fix: #1840 release the idempotency key on a definite failure #2372 before merging, I can add the corresponding guard to failExecution (treating params.transactionHash && params.chainId === undefined as unconfirmed rather than failed), or we can leave fix: #1840 release the idempotency key on a definite failure #2372 as-is since reachability is zero in practice. Happy to follow your lead on what's cleanest for merge.

4. Release vs. Refresh

The Refresh approach (updating responseBody and responseStatus on the existing record through the finalize fence without deleting) is remarkably clean:

• Zero deletion surface.
• Zero double-broadcast risk.
• Cures the stale unconfirmed replay for both successful reconciliations and failures alike.
• As you noted from the MCP tool contracts, agents are already instructed to poll GET /api/execute/{executionId}/status rather than re-sending on unconfirmed, so keeping the record held is safe.

Standing down on all branch work for #2373 until you return with the core team's decision on Release vs. Refresh.

[suisuss]

Correcting one thing I left open, because it has been settled and it changes the answer to the question you asked.

I said I could not identify a live producer of hash-without-chainId and to treat the reachability as UNSURE. There is one. plugins/web3/steps/transfer-spl-token-core.ts:514-520 returns transactionHash with no chainId on its result type, and the isTransactionResult gate at app/api/execute/node/route.ts:174-188 requires only a string transactionHash and a string gasUsed - both present - so completeExecution receives chainId: undefined by a reachable path today.

That resolves your question on #2372 in the direction you were leaning away from. Had completeExecution kept returning failed there, node/route.ts:336 would map it to release and free the key for an SPL transfer that succeeded. So the unconfirmed return is load-bearing rather than defensive, and I would not describe the reachability as zero in the PR description. You do not need the symmetric guard on failExecution for this - I traced every hash-bearing failure return across the four chain-write routes and the whole plugins/*/steps/*.ts surface, and each one pairs the hash with a numeric chainId in the same ternary, so that hole is type-level with no producer I could find. Worth a comment saying it rests on convention rather than the type.

One consequence to file separately rather than carry on your branch: the unconfirmed row that now results has a hash, no receipts and a null completedAt, so it passes the reconciler's eligibility filter, resolveChainId falls through to Number(execution.network), and the node route stores the caller's raw config string - Number("solana") is NaN, so reconcileOne stamps a terminal failed with "chain could not be resolved" without ever reading a receipt. A successful Solana transfer ends up recorded as failed. The fix is a chainId on that core's success return, which is nothing to do with idempotency.

Still working the release-against-refresh question with the team and will come back here with it. Nothing on this issue is blocked on you.