improve(binance): never retry a rate-limit response

[droplet-rl]

Binance rate-limit responses are currently retried like any other failure. getBinanceWithdrawals / getBinanceDeposits / getBinanceFillTrades call the SDK's generic retry(fn, 3, 2), whose delays are 2 ** i + random — roughly 1s, 2s, 4s. So a throttle gets three more requests inside ~7 seconds, which is what escalates it into an HTTP 418 IP ban.

The change: don't retry a rate-limit response.

if (isBinanceRateLimitError(err) || attempt >= maxRetries) {
  throw err;
}

isBinanceRateLimitError matches error code -1003 / -1015, or HTTP status 429 / 418. Non-rate-limit failures keep exactly their current retry behaviour.

Why not honour Retry-After and sleep instead? Because we can't read it. binance-api-node@0.12.7 builds the error from the response body when that body is JSON — which it is for rate-limit responses — and keeps only code, discarding the Response and its headers:

try {
  const json = jsonBigint.parse(text);
  error = new Error(json.msg || `${res.status} ${res.statusText}`);
  error.code = json.code;          // no res, no headers
} catch (e) {
  error = new Error(`${res.status} ${res.statusText} ${text}`);
  error.response = res;            // only for non-JSON (proxy) errors
}

Two consequences: Retry-After is unavailable on the responses that matter, and an HTTP 418 ban arrives looking identical to a 429 throttle. So sleeping-then-retrying would be guesswork that risks retrying a ban. Not calling again is both simpler and stricter. The 429/418 status checks are kept for the non-JSON proxy-error path, where the status is retained.

The trade-off is that a throttled request fails its run rather than recovering in-place. These three are history/balance reads on scheduled bots, so the next run picks them up — and failing fast also keeps a throttled dependency from stretching a run past its own interval.

Verification — yarn tsc --noEmit, eslint and prettier clean; 52 Binance tests pass (BinanceUtils, BinanceClient, BinanceFinalizer, BinanceAdapter.withdrawals, BinanceStablecoinSwapBridge). Four new unit tests cover both error shapes, the negative cases (-1021 clock skew, 502), that a rate limit is not retried, and that ordinary failures still are.

Not addressed here

• Retries remain silent — the SDK helper swallows intermediates with a bare .catch(), so request volume during an outage is invisible. BinanceUtils has no logger to thread; separate change.
• No per-attempt timeout, so a hung socket can still stall a run. Separate change.

_{Supersedes the first version of this PR, which added a shared cross-caller cooldown, a backoff budget and an error class. Codex flagged three P1s that were all artefacts of that cooldown, plus the Retry-After/418 problem above — dropping the cooldown removes the first three and the client behaviour rules out the fourth.}

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-04T13:52:51.186644Z	9cd809f	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9cd809fe13

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Preserve the full Retry-After in the shared cooldown

When Binance returns a Retry-After greater than 60 seconds—particularly for an HTTP 418 ban—this truncates the server's deadline before publishing it through setBinanceCooldown. The current request is rejected, but another caller or the next run in the same process resumes after only 60 seconds, while the IP is still banned, which can prolong the ban. Cap an individual caller's sleep or reject it via the backoff budget without shortening the shared cooldown deadline.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Recheck the shared cooldown after sleeping

With concurrent callers, the earliest waiter can wake, receive another 429, and extend binanceCooldownUntilMs while the remaining callers are still sleeping. Those callers proceed directly from this await to fn() using the stale deadline, so they issue a burst during the newly extended cooldown and can escalate the throttle into an IP ban. Re-evaluate the shared deadline after each sleep before making the request.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Preserve HTTP status when classifying SDK errors

For Binance's direct JSON error responses, binance-api-node@0.12.7 constructs an error from the response body's code and msg without retaining the Fetch Response, so an actual HTTP 418 commonly reaches this parser as only code: -1003. This classifies the ban as an ordinary retryable throttle, waits for the 30-second fallback, and calls Binance again—the behavior this change is intended to prevent. Preserve the HTTP status and headers at the client boundary, or otherwise ensure a body-only -1003 cannot cause an HTTP 418 to be retried.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Apply the shared cooldown to every Binance request

Publishing the cooldown here only affects calls routed through retryBinanceRequest, but the same module still sends accountCoins, tradeFee, depositAddress, allOrders, order, withdrawal, and withdrawal-quota requests directly; the rebalancer also retries order-book requests independently. If a wrapped history request establishes a cooldown and a subsequent run or sibling path invokes one of those endpoints, it contacts Binance during the cooldown and can extend the throttle or ban. Make the cooldown a client-wide request gate rather than applying it only to the three history/trade readers.

Useful? React with 👍 / 👎.

[droplet-rl]

I've used my 1 automated round on this PR, so I'm ignoring further automated-reviewer activity. To re-activate me: have a human @-mention @droplet-rl in a comment (resets the budget), or assign me to the PR (raises the budget to 3 rounds).

[droplet-rl]

All four P1s were real. Fixed in f4778ad6.

1. -1003 masking a real 418 (the important one). Confirmed against binance-api-node@0.12.7 rather than taken on faith. Its sendResult attaches the Fetch Response only on the branch where the body fails to parse as JSON (http-client.js:108); a JSON-parseable body takes the branch at :103 that sets code/msg/url and drops the response. Binance answers a real 418 with a JSON body, so the ban arrives as code: -1003 with no status and no Retry-After. I ran the actual error shape through the parser before changing anything:

real 418 ban -> {"retryAfterS":30,"banned":false}
real 429     -> {"retryAfterS":30,"banned":false}

Indistinguishable — so banned was dead code in production and every real ban was retried. Exactly the behaviour the PR set out to prevent, which makes my "gate on code/status, never message text" claim in the description wrong: for the JSON shape there is no status. Bans are now recognised from the deadline Binance states in the message (banned until <epoch ms>), which also yields the true expiry instead of the 30s guess. Classification stays gated on code/status; the message is read only to recover the deadline. Flagging the reversal explicitly since it contradicts what I wrote in the PR body.

2. Truncating the shared cooldown. Agreed, and it compounded #1 — the 60s cap was applied to the value being published, so even a correctly-detected ban would release every caller after a minute. Per-sleep capping and the shared deadline are now separate: sleeps happen in ≤60s slices, the cooldown carries Binance's real deadline. Kept a 1h ceiling so a malformed deadline can't wedge the process indefinitely; past that we re-probe and get re-banned if still out.

3. Waking against a stale deadline. Correct — the wait was a single if, then straight through to fn(). Now a loop that re-reads the deadline after every sleep. Test drives it: a sibling extends the cooldown mid-sleep and the caller bails instead of firing.

4. Cooldown only covering three readers. Right, and there were two client construction sites, not one — getBinanceApiClient and BinanceClient.create. Both now wrap the client in a Proxy that routes all 150 request methods through the gate, so privateRequest, accountCoins, tradeFee, book, exchangeInfo and the rebalancer's order-book path are covered, as are endpoints added later. Mutating calls (order, withdraw) are gated but deliberately never retried. ws is the only non-function member and passes through ungated — websockets aren't REST-weighted, and Binance recommends them to avoid these limits.

One thing that came out of composing #4 with the retry loop: a gate refusal (BinanceRateLimitError) has no code/response, so the outer loop would have read it as transient and retried it. Now non-retryable, with a test.

Verification — tsc --noEmit, eslint and prettier clean; 98 Binance tests pass (36 in BinanceUtils incl. 6 new, 62 across BinanceClient/BinanceFinalizer/BinanceAdapter.*/BinanceStablecoinSwapBridge/swapOnBinance/sweepBinanceBalance). New coverage: real JSON ban and throttle shapes, uncapped-vs-clamped cooldown, mid-sleep re-read, client-wide gating, gate refusal not retried.

Known looseness, not fixed: a gated client method called inside retryBinanceRequest passes through two gates, each with its own 90s budget, so a worst-case retrying read can wait ~180s rather than 90s. Only reachable if a sibling extends the cooldown in the window between the outer wait and the inner check, and it costs local latency rather than extra requests to Binance — the gate still blocks the call. Threading one budget through would need plumbing I didn't think was worth it here; happy to if you'd rather have the hard bound.

Also unchanged from the original description: retries are still silent (no logger in BinanceUtils), and there's still no per-attempt timeout. Both worth follow-ups.

That's the single automated Codex round used up. The four findings were well-targeted — if you want another pass over this, assigning me to the PR raises the budget.

[droplet-rl]

🔎 View trace

[pxrl]

Do better - don't just assert types like this. Use superstruct.

[droplet-rl]

Done in 90bf1d62.

const BinanceApiErrorSS = type({ code: number() });
const BinanceHttpErrorSS = type({ response: type({ status: number() }) });

export function isBinanceRateLimitError(err: unknown): boolean {
  if (is(err, BinanceApiErrorSS) && BINANCE_RATE_LIMIT_CODES.has(err.code)) {
    return true;
  }
  return is(err, BinanceHttpErrorSS) && BINANCE_RATE_LIMIT_STATUSES.has(err.response.status);
}

Two structs matched independently rather than one struct of optional fields. That turned out to matter: with a single type({ code: optional(number()), response: ... }), a malformed code fails validation for the whole error and takes the valid response.status down with it. So { code: "-1003", response: { status: 429 } } would read as not-rate-limited and get retried — which the assertion also got wrong, since it short-circuited on typeof code === "number" before looking at the status. Both shapes now stand on their own, with a test pinning it.

type() over object() so message/stack/url are ignored rather than rejected — same reasoning as the comment on WithdrawalQuotaSS in BinanceClient.ts.

Also added coverage for the shape binance-api-node actually throws (code set on an Error instance, not a plain object) and for wrong-typed signals. 30 tests pass in BinanceUtils, 62 across the other Binance suites; tsc and lint clean.

[droplet-rl]

Thanks — addressed in 90bf1d62, replied inline on the thread.

Swapped the as assertion in isBinanceRateLimitError for superstruct, following the WithdrawalQuotaSS pattern in BinanceClient.ts (type() over object() so the rest of the error is ignored rather than rejected).

Worth flagging one thing that came out of it: I used two structs matched independently rather than a single struct of optional fields. With one struct, a malformed code fails validation for the whole error and takes a valid response.status down with it — so { code: "-1003", response: { status: 429 } } would come back as not-rate-limited and get retried. The original assertion had the same hole from the other direction, short-circuiting on typeof code === "number" before reaching the status. Both shapes now stand alone, with a test pinning it.

Also added coverage for the shape binance-api-node actually throws — code assigned onto an Error instance rather than a plain object — since that's what the predicate sees in production and none of the existing cases exercised it.

tsc --noEmit, eslint and prettier clean; 30 tests in BinanceUtils and 62 across the other Binance suites pass.

One note for the record: this branch was force-pushed to 25a8958c since my last pass, which dropped the commit I'd pushed there. No objection — the "never retry, the caller runs again anyway" framing is simpler than the cooldown machinery I'd built, and it sidesteps the per-IP shared-state and budget questions entirely. The binance-api-node finding is preserved in the comment at the top of the block, which is the part that actually mattered. Just noting it in case the force-push wasn't deliberate.

[droplet-rl]

🔎 View trace