KeeperHub / KeeperHub/keeperhub
invokeStep ignores a step's declared maxRetries, so an irreversible step can be retried by request
- 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
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