Repository navigation
feat(client): cap the requests in flight at 50 per client - #164
MarketDataDev02 wants to merge 5 commits into
Conversation
SDK requirements section 12 asks for one client-level pool of 50 requests, shared by every endpoint, that hands a freed slot to the next request at once. stocks.candles and options.quotes each built their own 50-worker executor per call, so two calls running at the same time put 100 requests on the wire and the API answered the excess with 429. MarketDataClient now owns a BoundedSemaphore(50) that _make_request holds around the HTTP exchange only. Every request of the client draws from it, the fan-outs keep their executors, and a retry sleeping through its backoff holds no slot. A request that finds none waits for one.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #164 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 62 62
Lines 3297 3300 +3
=========================================
+ Hits 3297 3300 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ Ready — Code reviewVersion impact: minor (1.3.0 -> 1.4.0). The change adds a fixed per-client 50-request sliding-window limit without changing a public signature, default, type, or exception contract, and documents and tests the behavior across all output formats. The same per-call rather than per-client pooling defect exists in sdk-php and is tracked as MarketDataApp/sdk-php#104; sdk-go, sdk-csharp, sdk-java, and sdk-js already use client-level pools. Rating patch 6/6 (exemplary) · proof 6/6 (comprehensive) · overall 6/6 Since last review
Reviewed |
The failure, backoff and second-client cases read the private slot count of the client. They now saturate the pool with public calls instead: the calls waiting behind requests that fail still run, fifty retries asleep in their backoff leave room for another call, and a second client is not held up by a client whose slots are all taken.
The live runs of the pool did not see the API refuse requests above 50 in flight, so README and CHANGELOG state the limit the API documents instead of saying it enforces it with a 429.
The pool sits before any output-format code, so the format cannot change how many requests are in flight. A scenario per format checks it through the public client: with the pool full, ten more calls wait and every call is served on INTERNAL, JSON, CSV and DATAFRAME.
marketdata-code-review
Bot
added
review: ready
and removed
review: needs changes
labels
Closes #65
Summary
SDK requirements §12 asks for one client-level pool of 50 concurrent requests, shared by every endpoint, as a sliding window: a slot frees and the next request starts. The API's rate-limit docs say it refuses requests beyond 50 in flight with a
429(rate limits).stocks.candles()andoptions.quotes()each built their ownThreadPoolExecutor(max_workers=50)per call, and nothing bounded the total. Two calls running at once from two threads put 100 requests on the wire, and 120 threads making one call each put 120.MarketDataClientnow owns athreading.BoundedSemaphore(50), and_make_requestholds one of its slots around the HTTP exchange. Every request of the client draws from that one budget, a request that finds none waits for a slot, and the fan-outs keep their executors for parallelism.sdk-go, sdk-csharp, sdk-java and sdk-js keep one pool per client too, and none of them puts a time limit of its own on the wait. They differ on retries: sdk-csharp, sdk-java and sdk-js take the slot per attempt, as this PR does, while sdk-go takes it once per call and holds it through the backoff. sdk-csharp makes the size configurable (1 to 50); here it is fixed, as the issue asks. sdk-php has no client-level pool: only each parallel call is limited to 50 (see Review round 1; tracked in MarketDataApp/sdk-php#104).
Version impact
minor (1.3.0 -> 1.4.0) on its own: no public name, signature, default or exception changes. The next release is 2.0.0 either way, since
mainalready carries breaking changes, and the CHANGELOG entry is under Changed, not BREAKING. What a caller sees change:429, which the SDK never retries, so those calls raisedRateLimitError. The live runs of Review round 1 did not reproduce that refusal.The change
client.py:__init__creates_request_slotsbefore_setup_rate_limits, whose/user/request already takes a slot._make_requestholds a slot aroundself.client.request(...)in awith, so an exception releases it too. That is before any output-format code runs, so the output format cannot change whether a request waits or how many are in flight.test_the_output_format_does_not_change_what_the_pool_allowsruns the saturation scenario on INTERNAL, JSON, CSV and DATAFRAME through the public client.__init__and_make_requestgain docstrings, and__init__loses an issue number in a comment.src/tests/test_request_pool.py: thirteen cases, below.README.md,docs/stocks.mdanddocs/options.mdsaid "up to 50 concurrent requests" per call. They now describe the cap as the client's 50 slots, per client and not configurable. The CHANGELOG entry says the same. README and CHANGELOG call the limit the one the API documents, which it states answers with a429, because the live runs of Review round 1 did not see that refusal.Choices:
NetworkErrorit would be retried and queued again.429(the sameRateLimitError) instead of the local refusal./status/uses the pool. The cap stays exact. A stale status cache reads asunknownand the retry goes on (§9.5), so a full pool delays the refresh and blocks nobody._make_requestruns once per attempt on every path: single requests (the decorator retries the whole method), fan-outs (each request retries on its own) and the start-up/user/. tenacity sleeps between attempts, outside the slot, so a retry waiting out its backoff holds none.Proof
What main does. On main
c4ee8e1, throughMarketDataClientover a mocked transport that holds each request for 0.25 s, the peak of requests in flight:The first is
options.quotesover 60 symbols andstocks.candlesover 60 years of hourly candles (61 yearly chunks), started together from two threads. The second is 120 threads making onestocks.pricescall each.After. The same script on this branch:
Every request is still served, and none errors. The script is not part of the PR; its two scenarios are the first two tests below, which read the peak from a gate instead of a clock.
Suite.
uv run pytest -n 4 --cov="marketdata"on this branch merged with main49d331d, as CI runs it: 2177 passed, coverage 100% (3300 statements, 0 missed). Thirteen of those tests are new. Onf3ccfdc, before the merge of main and the four output-format cases: 1988 passed, coverage 100% (3211 statements).The new tests on main. With main's
src/marketdatachecked out on this branch, 11 of the 13 fail, all on an assertion: the three saturation tests (120 requests in flight at once, the pool allows 50,100 requests in flight at once, the pool allows 50andassert 51 == 50), the four output-format cases (60 requests in flight at once) and the four failure cases (assert 60 == 50). The backoff and second-client tests pass on main, which has no pool and so never holds a call up. They pin what the pool must not do, and the mutations below turn them red. On main49d331d, which the branch is now merged with, the same 11 fail with the same numbers.Mutations. Each applied to the branch and restored after:
finally: 3 fail, the timeout, connection-error and unexpected-exception cases. The 5xx case returns normally from the transport, so it stays green by design.api_error_handler: 12 fail,test_retries_asleep_in_their_backoff_do_not_hold_up_another_callamong them. The two-fan-outs test stays green, since the fan-outs retry each request on their own and do not go through that code.test_a_client_with_every_slot_taken_does_not_hold_up_another_clientfails.Live.
src/tests/integration -m integrationon 2026-10-02 at 18:47Z: 20 passed. The paired bursts are in Review round 1.Tests
src/tests/test_request_pool.py, through the public client withrespx, with no test reading the pool's private state. A gate holds every request inside the mocked transport and counts how many are there, so each count is read at rest instead of from a clock. Every wait has a timeout, and the caller threads are daemons, so a regression that deadlocks fails the test instead of hanging the run.test_120_callers_never_put_more_than_50_requests_in_flight: 120 threads, onestocks.pricescall each. 50 requests are in flight and 70 wait, the peak never passes 50 and all 120 are served.test_two_fan_outs_started_together_share_the_pool: anoptions.quotesand astocks.candlesstarted together on a barrier. They draw from one budget of 50, across endpoints.test_a_freed_slot_starts_the_next_request_without_waiting_for_the_others: with 50 in flight and one waiting, one completion starts the waiting request while the other 49 still run. That is the sliding window, not batches.test_the_output_format_does_not_change_what_the_pool_allows, on INTERNAL, JSON, CSV and DATAFRAME: 60stocks.pricescalls with the pool full, 50 in flight and 10 waiting, and every call served. On CSV each call gets its own filename, since the file is created exclusively.test_calls_waiting_for_a_slot_go_on_when_the_requests_ahead_fail, for a timeout, a connection error, an unexpected exception and a 5xx after its retries: 60 calls, the pool full of requests the gate then fails, and the 10 behind them still run and fail in turn. A slot lost to a failure leaves them waiting.test_retries_asleep_in_their_backoff_do_not_hold_up_another_call: 50 calls refused once with a 503 and asleep in their backoff, and one more call goes through meanwhile. A slot held through the sleep blocks it. The/status/refresh runs inline, so no helper thread takes a slot.test_a_client_with_every_slot_taken_does_not_hold_up_another_client: a client whose 50 slots are all taken leaves a second client's call free to go through.Review round 1
Revision 1 of the bot, on
0d35dff: Blocked, patch 5/6, proof 4/6, with one proof item and two should-fix findings.Paired live runs (proof). Each run uses one
MarketDataClient, 120 threads released together on a barrier, and onestocks.prices("AAPL")call per thread (a free-trial symbol), first on mainc4ee8e1and then on this branch. A counting transport under the client records what the API answered on the wire and how many requests the SDK had sent and not yet seen answered. In every row the 120 calls made 120 requests, and there was no transport error.429, the measuring client's connection limit was raised to 200 (httpx's default of 100 holds part of the 120 back) and a wave of 120 opened the connections, so that the measured wave could reuse them and reach the API together. The limit is the same for both versions and the SDK is unchanged.429203x 1200d35dff203x 120203x 120203x 120f3ccfdc203x 120f3ccfdc203x 120f3ccfdcchanges tests only, so the runtime code of both branch runs is that of0d35dff. The peak on main is what the SDK asked the API to carry at once. The API answered every request with a cached203and refused none, in both runs, so neither reproduced the429. These runs cannot tell whether the API ever had more than 50 requests in flight at once, or whether it counts cached203answers against its limit. The429above 50 stays as the rate-limit pages document it, and was not seen. The scripts are not part of the PR.Tests that read private state (
91f5a4e8).f3ccfdcreplaces the six cases that counted the pool's slots (the four failure cases, the backoff case and the second-client case) with scenarios through the public client, each from a full pool. They are described under Tests. No test reads_request_slotsnow, and the suite is the same 1988 passed with 100% coverage.Sibling pools (
a053dd93). Read from each repository'smain:HttpDispatcherperMarketDataClientholds a 50-permitAsyncSemaphore(HttpTransport.CONCURRENCY_LIMIT, fixed). A permit is taken per attempt and released exactly once, on success, failure or cancel, and the backoff runs outside it. The only way out of the wait is cancelling the returned future. The credit pre-flight runs before the wait, as here. It matches §12.client.tskeeps onepLimit(50)per client (MAX_CONCURRENT_REQUESTS), and_executeWithRetrycalls it inside eachpRetryattempt, so a backoff holds no slot. The queue has no bound. The credit pre-flight runs inside the pool, after the slot, as in sdk-go. It matches §12.ClientBase::execute_in_parallel, whichOptionsandStocksuse for their fan-outs, runs a GuzzleEachPromisewithconcurrency50, a sliding window per call. Two such calls running at once can pass 50, the shape sdk-py had. A call waits for all its requests before it returns, so two of them overlap only if the program runs them concurrently itself (fibers, an event loop) or in separate processes, which no per-client pool can bound. It is the one sibling without the client-level pool §12 asks for, and it is tracked in The 50 request limit applies per execute_in_parallel call, not per client (SDK requirements §12) sdk-php#104.CI and main
The live job of
f3ccfdcfailed with 1 test of 20:test_status_lists_every_servicegot a500fromGET /status/at 2026-10-02 19:32:48Z, a Cloudflare page titled "Worker threw exception | api.marketdata.app", which the SDK raised asInternalError. The same job passed on0d35dff, which has the same runtime code; the unit matrix on 3.10, 3.11 and 3.12 and Codecov were green onf3ccfdc; and/status/answered200on 2026-10-13. The red gate kept the bot from reviewingf3ccfdc.This push merges main
49d331d(17 commits, #159 and #165 among them, no conflicts) and re-runs the whole CI, the live job included. On the merged tree: 2173 passed, coverage 100% (3300 statements, 0 missed), and the new tests fail on main as before. README and CHANGELOG now call the limit the one the API documents, since the live runs did not reproduce the429.Review round 2
Revision 2 of the bot, on
7d10cd4: Needs changes, patch 5/6, proof 6/6, overall 5/6. It resolved91f5a4e8anda053dd93and raised two should-fix findings.Track the sibling pool defect (
c27b5431). MarketDataApp/sdk-php#104 records that sdk-php limits eachexecute_in_parallelcall to 50 and not the client. It carries the code references, the comparison with the other SDKs, and what the live runs did and did not show. Nothing changes in this PR for it.Cover all four output formats (
91b4cd2d).f6c6878addstest_the_output_format_does_not_change_what_the_pool_allows, which runs the saturation scenario through the public client on INTERNAL, JSON, CSV and DATAFRAME: 60 calls, 50 in flight and 10 waiting, and every call served. The slot is taken before any output-format code runs, so the four formats reach the pool by one path, and the test checks that instead of asserting it. On main the four cases fail on their assertion, and removing the semaphore turns them red along with the rest.Suite after the round: 2177 passed, coverage 100% (3300 statements, 0 missed).