KeeperHub / KeeperHub/keeperhub

invokeStep ignores a step's declared maxRetries, so an irreversible step can be retried by request

Open
#2,498 2 comments 0 reactions 0 assignees View on GitHub
accepted bug confirmed
Dominant language
TypeScript
Stars
24
Forks
93
Avg merge
1d 4h
Merged PRs (30d)
253

Description

Found while reviewing #2438, which declares `maxRetries = 0` on a step that spends money and does not get it.

**Reason.** A step function can declare `stepFn.maxRetries = 0`, and the DevKit workflow runtime honours it. `app/api/execute/node/route.ts` does not: `invokeStep` at `:216-240` branches only on the caller's `retry` config and on `isWeb3`, and never reads `stepFn.maxRetries`. So a step author's "this must never be retried" is silently ignored on the direct-execution route.

It matters because of what sits downstream. `isWeb3` is `Boolean(network)` at `:450`, so any action exposing a `network` field takes the `transactionRetryOptions` branch. There, `TX_ERROR` (`app/api/execute/_lib/retry.ts:82-89`) returns the error string for any failure carrying no `transactionHash`, and `isRetryableError` (`:188-194`) matches `timeout`, `ETIMEDOUT` and `ECONNRESET` as case-insensitive substrings. A non-web3 step whose failure text happens to contain any of those words is retried, whatever it declared.

The concrete case: the x402 paid-resource step in #2438 delivers an `X-PAYMENT` header, the server settles the payment, the connection then resets before the response returns, and the step's error string carries `ECONNRESET`. `executeWithRetry` re-runs the whole step with the same signature, up to `maxRetries + 1` times. An EIP-3009 nonce blocks a second on-chain settle, but the resource is re-requested and re-metered, and any off-chain-metered rail double-pays outright.

`retry.ts:124-130` already documents this hazard for web3 broadcasts and gates on the transaction hash. Steps with an irreversible side effect and no hash have no equivalent gate.

**Scope.** `app/api/execute/node/route.ts` `invokeStep`, and whatever type `StepFn` needs to carry the declared value. Does not touch the DevKit runtime, which already honours it, or the four chain-write routes, which do not go through `invokeStep`.

Out of scope: widening or narrowing `RETRYABLE_PATTERNS`, and the `isWeb3` derivation itself - both are worth looking at, neither is needed to close this.

**Plan.** Read `stepFn.maxRetries` in `invokeStep` and clamp the caller's `retry.maxRetries` to it when the step declares one, so a step that says zero is invoked exactly once regardless of what the request asked for. A caller asking for more retries than a step permits is not an error worth rejecting - silently honouring the lower bound matches how the runtime already behaves.

Worth deciding in review, not settled here: whether a step declaring `maxRetries` should also opt out of the `isWeb3` branch entirely, since a non-web3 step being classified by `TX_ERROR` is the second half of why this bites.

Contributor guide

Open the contributing guide

Research direction

Start with invokeStep at app/api/execute/node/route.ts:216-240 and locate the StepFn type that carries step-declared retry settings. Read the retry behavior in app/api/execute/_lib/retry.ts, then verify that the caller cannot exceed a declared maxRetries value, including zero, while existing web3 retry handling remains unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
api, backend, blockchain
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.