KeeperHub / KeeperHub/keeperhub

Retry budget check uses a default of 0 where the executor defaults to 3, admitting 4x the intended ceiling

Open Beginner friendly
#2,513 0 comments 0 reactions 0 assignees View on GitHub
accepted bug confirmed
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.