KeeperHub / KeeperHub/keeperhub
Retry budget check uses a default of 0 where the executor defaults to 3, admitting 4x the intended ceiling
- Dominant language
- TypeScript
- Stars
- 24
- Forks
- 93
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 266
Description
Found while reviewing #2507, whose fix incidentally closes this for part of the surface and leaves it open for the rest.
**Reason.** `app/api/execute/node/route.ts:83` computes the retry budget as `const attempts = ((r.maxRetries as number | undefined) ?? 0) + 1;` and checks `attempts * perAttempt > MAX_RETRY_BUDGET_MS`. But `resolveConfig` in `app/api/execute/_lib/retry.ts` defaults a missing count to `DEFAULT_MAX_RETRIES = 3` (`retry.ts:5`), not to 0.
So a request of `retry: { timeoutMs: 600000 }` with no `maxRetries` is validated as one attempt of 600000ms - inside the budget - and then executed as four attempts, up to 2,400,000ms. The budget check is the thing that is supposed to keep a request inside the idempotency processing lock (`lib/idempotency.ts` `PROCESSING_TTL_MS`, 10 minutes), and the comment above the check says so. Four times that lock is the exact condition it exists to prevent: the lock expires while the request is still running, and a concurrent retry under the same key can reserve fresh.
**Scope.** The single expression at `route.ts:83`. No change to `resolveConfig`, to `executeWithRetry`, or to the budget constant.
Note the interaction with #2507, which is why this is worth doing now rather than later: that PR's `capRetriesByDeclaration` uses `config.maxRetries ?? DEFAULT_MAX_RETRIES`, so for any step declaring `maxRetries` the effective count is clamped and the hole closes as a side effect. It stays open for the steps that declare nothing - `chainInfoStep`, `erc20BalanceStep`, `ethBalanceStep`, `gasPriceStep` and the other undeclared ones, several of which take a `network` and therefore sit on the retrying branch. Fixing the validator is what makes the guarantee independent of which steps happen to declare a ceiling.
Out of scope: whether `DEFAULT_MAX_RETRIES` should be 3 at all on this route, and whether the budget should be measured against the lock TTL dynamically rather than a constant. Both are reasonable questions and neither needs answering to fix the arithmetic.
**Plan.** Use the same default the executor uses, so the check models what will actually run. Importing `DEFAULT_MAX_RETRIES` from `retry.ts` rather than restating the literal is the version that cannot drift again - it is currently module-private, so it needs exporting. A test sending `retry: { timeoutMs: 600000 }` and expecting a 400 pins it.
Contributor guide
Research direction
Start at app/api/execute/node/route.ts:83 and compare its retry-budget calculation with DEFAULT_MAX_RETRIES and resolveConfig in app/api/execute/_lib/retry.ts; also review the PROCESSING_TTL_MS context in lib/idempotency.ts. Add a regression test for retry: { timeoutMs: 600000 } with no maxRetries, and confirm the request is rejected with HTTP 400 while existing retry behavior remains unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- api, backend
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 88/100