feat(brokers): retry robinhood GETs on 429 and 5xx, never POSTs - #420
Merged
Conversation
Robinhood allows 100 requests/minute sustained and 300 in a burst, and this transport had no backoff at all -- a 429 propagated as a bare HTTPError alongside 401 and 5xx. The limit is reachable in ordinary use, not just under abuse: `get_fee_summary` alone can spend 21 requests in one call (`_MAX_PAGES` plus the account read), and `preview_order` now adds a trading_pairs read on top. A "wait half a second" was turning into a failed cycle. GETs are now retried up to `_MAX_ATTEMPTS` (4) on 429 and 5xx, doubling from 0.5s and capped at 8s. `Retry-After` wins where the venue sends one -- it knows when its window resets and we do not -- but is CLAMPED, not obeyed: `Retry-After: 3600` is a legal response, and honouring it literally would park a trading loop for an hour inside what the caller believes is a bounded read. Only the delta-seconds form is read; the HTTP-date form is legal and deliberately ignored, because honouring it means trusting the server's clock against ours while the fallback is already correct without any clock. A POST IS NEVER RETRIED, and that is the decision rather than the gap. `create_order` is the only one, and a 429 or 5xx on it is an UNKNOWN outcome, not a refusal -- the venue may have accepted the order before the response was lost. Sending it again places a second live order unless both attempts carry the same `client_order_id`, and this layer cannot tell whether they would: the body arrives already built, and an id derived from a caller's idempotency key (#409) is indistinguishable from a fresh uuid4 from here. Now that #409 exists the caller CAN make a placement retry safe, so the retry belongs where that knowledge lives -- above the adapter. A transport that retried POSTs would be guessing, on the one request where guessing costs money. 404 stays unretried: it is an ANSWER, and the 404-to-None split remains exactly as load-bearing as before. 401 stays unretried: a signature or clock problem does not heal in half a second, and retrying turns one clear failure into four identical ones. A retryable status that survives every attempt still raises, so a persistent quota problem stays visible rather than becoming a hang. Signing moved INSIDE the retry loop. The signature is valid for 30 seconds from its timestamp, so a retry that waited out a backoff would otherwise present a stale one and come back 401 -- a failure invented by retrying, and indistinguishable from a bad credential. Pinned by a test that advances the clock between attempts, because two attempts in the same second sign identically and would prove nothing. `sleep` is injected so the retry path is testable without a suite that actually sleeps, and `_transport()` in the tests defaults it to a no-op -- otherwise every existing test touching a 429 or 5xx would have waited out real backoff. No jitter: jitter desynchronises a herd, and this is one process making one request at a time against one account. Closes #411. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
eaitbrahim
added a commit
that referenced
this pull request
Aug 20, 2026
Minor, not patch. Three things since v0.9.3 change what an implementor or a deployment can rely on: * A NEW DISTRIBUTION. `keel-broker-alpaca` (#382, #384) plus the paper-equities profile that selects it (#386), so a deployment can now be US equities via the broker port rather than crypto only. * THE PORT CONTRACT MOVED TWICE. `market_clock`/`market_schedule` made venues session-aware (#385), and `place_order` gained `idempotency_key` (#419). Both carry defaults so no CALLER breaks, but a third-party adapter that does not accept them is no longer a `Broker` -- the conformance suite now says so. That is exactly the kind of change a patch bump must not hide. * THE OPERATOR CONSOLE. The TUI became keel's console across #399-#408, and `keel update` (#415/#417) makes a deployment self-updating. Every pinned sibling moves with it. The four production distributions are required `==` at this exact version (`RELEASING.md`, "Release assets"), so a bump that missed one would install a mixed set -- the `keel-trader 0.5.7` against `keel-core 0.5.5` failure `keel versions` exists to catch, and which `~/keel` actually ran across two releases. Also in this window, on the Robinhood adapter: the best_bid_ask fixture corrected against the live venue (#414), a credential guard that catches the error it only claimed to (#416), pre-flight sizing reported on the preview (#418), transport backoff (#420), and the fenced one-order probe (#421). Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #411. Unblocked by #419 — see the POST section.
The gap
Robinhood allows 100 req/min sustained, 300 burst, and the transport had no backoff at all: a 429 propagated as a bare
HTTPErroralongside 401 and 5xx.The limit is reachable in ordinary use, not just under abuse.
get_fee_summaryalone can spend 21 requests in one call (_MAX_PAGES+ the account read), andpreview_ordernow adds atrading_pairsread on top (#418). A "wait half a second" was turning into a failed cycle.What retries
Nonesplit stays as load-bearing as everRetry-Afterwins where the venue sends one, since it knows when its window resets and we do not — but it is clamped, not obeyed.Retry-After: 3600is a legal response, and honouring it literally would park a trading loop for an hour inside what the caller believes is a bounded read.Only the delta-seconds form is read. The HTTP-date form is legal and deliberately ignored: honouring it means trusting the server's clock against ours, and a skewed clock gives either an instant retry (useless) or a very long sleep (worse), while exponential backoff is already correct without any clock at all.
A retryable status that survives every attempt still raises, so a persistent quota problem stays visible rather than becoming a hang.
Why a POST is never retried
This is the decision, not the unfinished part.
A 429 or 5xx on
create_orderis an unknown outcome, not a refusal — the venue may have accepted the order before the response was lost. Sending it again places a second live order unless both attempts carry the sameclient_order_id, and this layer cannot tell whether they would: the body arrives already built, and an id derived from a caller's idempotency key (#409) is indistinguishable from a freshly minted uuid4 from here.Now that #409 exists, a caller can make a placement retry safe. So the retry belongs where that knowledge lives — above the adapter, not inside the transport. A transport that retried POSTs would be guessing, on the one request where guessing costs money.
test_a_POST_IS_NEVER_RETRIED_even_on_a_429asserts one call and zero sleeps, with the reason in the docstring.Signing moved inside the loop
The signature is valid for 30 seconds from its timestamp. A retry that waited out a backoff and presented the first attempt's signature would come back 401 — a failure invented by retrying, and indistinguishable from a bad credential.
Pinned by a test that advances the clock between attempts, because two attempts in the same wall-clock second sign identically and would prove nothing.
Testability
sleepis injected, and_transport()in the tests defaults it to a no-op. Without that, every existing test touching a 429 or 5xx would have waited out real backoff — the first run of this change took 11.8s instead of 1.3s for exactly that reason.No jitter: jitter exists to desynchronise many clients retrying in lockstep, and this is one process making one request at a time against one account. Noted in the docstring so the reasoning expires visibly if that ever changes.
Still true, and still in the README
Backoff is not throttling. The aggregate rate is still unbounded, and
get_fee_summary's 1 + N sweep should be called on a schedule, not per order.Full suite: 3777 passed, 3 skipped.
ruff checkandmypy(190 files) clean.🤖 Generated with Claude Code