feat(api): reuse connections and retry transient API failures - #1340
Draft
davidberenstein1957 wants to merge 2 commits into
Draft
feat(api): reuse connections and retry transient API failures#1340davidberenstein1957 wants to merge 2 commits into
davidberenstein1957 wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## scaling/01-emission-timestamps #1340 +/- ##
==================================================================
+ Coverage 91.44% 91.51% +0.07%
==================================================================
Files 49 49
Lines 5061 5104 +43
==================================================================
+ Hits 4628 4671 +43
Misses 433 433 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ApiClient called module-level requests functions with a hardcoded timeout=2 and no retry policy, so every emission POST opened a fresh connection and any transient failure discarded the measurement. - Add a Session with an HTTPAdapter/urllib3 Retry (backoff + jitter, falling back gracefully on urllib3 < 2.0). POST is deliberately retried: a duplicate telemetry row beats a lost measurement. - Make timeout, retries and backoff constructor arguments, exposed to users as the api_timeout / api_retries config knobs. - Back off run creation after a failure instead of re-attempting it on every measurement tick, which otherwise stampedes the API after an outage. - Close the Session in CodeCarbonAPIOutput.exit() so long-lived processes do not leak sockets. Measured against a loopback stub server: 50 sequential emission POSTs open 1 TCP connection instead of 50, and a row that hit two transient 503s is now delivered instead of dropped. Worst-case time an unreachable API can block the measurement loop is ~16s, below the tracker's own 45s stale-measurement warning. The disk spill buffer from the plan is deliberately not implemented. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
status_forcelist included 502/503/504 and read=retries applied to POST, so a 504 or a read timeout arriving after carbonserver had already committed the insert made the client send the row again. There is no idempotency key and the dashboard sums emission rows, so that silently inflates a user's reported emissions -- and unlike a dropped row, nothing shows it happened. - Split the retry policy in two. GETs and PATCHes keep the broad policy. POSTs only retry failures that plausibly never reached the application: connection errors, connect timeouts, 429/502/503. Read timeouts, truncated responses, 500 and 504 are no longer retried. Widen only behind a server-side idempotency key. - Replace the 'telemetry, not billing' comment: wrong call for a carbon-accounting product. - Make the timing numbers agree and say which worst case is which. Measured: hung endpoint 5s (was 16.5s), unreachable endpoint 11s, worst case 16s, all under the tracker's 45s stale-measurement warning. - Document api_timeout / api_retries in docs/how-to/configuration.md. Tests: 4 new cases pinning POST no-retry on read timeout and on 504 against POST-still-retries on 503 and GET-still-retries on read timeout, plus one asserting api_timeout/api_retries reach ApiClient from the tracker constructor. All fail if the change is reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
davidberenstein1957
force-pushed
the
scaling/02-api-client-retry-and-timeouts
branch
from
August 12, 2026 17:53
a53dcb9 to
863b9c3
Compare
davidberenstein1957
changed the base branch from
master
to
scaling/01-emission-timestamps
August 12, 2026 17:53
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.
Part of #1338.
ApiClient._requestcalled barerequests.get/post/patchwith a hardcodedtimeout=2, noSessionand no retry policy, andhttp.py:_emitlogged failures away. This adds aSessionwith a jitteredRetryadapter, configurable timeouts, and a backoff guard around run creation.Stacked on #1341 — this branch is rebased on
scaling/01-emission-timestampsand its base is set to that branch, so #1341 merges first and this PR's diff stays limited to its own two commits. Both touchedapi_client.py; the conflict (the imports, and one function added next to the other) was resolved in the rebase by keeping both sides. Retarget tomasteronce #1341 lands.Measured against a loopback stub server (no real network)
The second row is the point: a transient 503 used to lose a measurement permanently. Two caveats stated plainly — the wall-clock win is over plain-HTTP loopback with no TLS handshake to save, so the connection count is the real measurement and the HTTPS-over-WAN latency benefit is inferred from it; and failure paths genuinely got slower, which is the deliberate trade for delivering the row.
POST is not retried once the request has landed
carbonserverhas no idempotency key onPOST /emissions, and the dashboard sums emission rows. So replaying a POST whose response was lost after the server committed the insert silently inflates a user's reported emissions — and unlike a dropped row, nothing shows it happened. For a carbon-accounting product that is the worse failure of the two.The retry policy is therefore split:
The POST column is exactly the set of failures where the request plausibly never reached the application. Widening it needs a server-side idempotency key first.
Timing budget (the two numbers that were inconsistent)
The earlier "16s worst case" and the docstring's
(3.05 + 5) * 3were describing different things and neither said which. Now stated separately, for the emission path's(3.05, 5)/retries=2budget:3.05 * 3plus jittered backoff.All well under the tracker's own 45s stale-measurement warning at
emissions_tracker.py. The CLI path keeps(3.05, 10)and the broad GET policy, so its hung-endpoint case is ~31s — also under 45s, and it is not on the measurement loop.Interaction with #1339 (scheduler)
This send runs inline on the scheduler thread, so a 5–16s blocking call is 5–16s the next tick is not taken. Today's chained-
Timerscheduler would let a slow send overlap the next measurement; once #1339 lands and ticks run on one thread, the same blocking call produces a skipped measurement instead of an overlapping one. Skipping is the better failure — it is why the budget is kept under the 45s warning rather than merely bounded — but the two PRs should be read together.Choices that differ from the original proposal
timeout=10, retries=3on the emission path; measured, that blocks a scheduler thread for 44s against a 15s tick. This uses(3.05, 5)/retries=2for emissions and(3.05, 10)/retries=2for the CLI.ApiClient._create_run, so both callers route through one fix rather than two.Configuration
api_timeout(default 5, read timeout in seconds) andapi_retries(default 2) are documented indocs/how-to/configuration.md, including which failures are retried on POST and why.Tests
tests/test_api_client_retry.py, 14 tests: retry-then-succeed on 503, give-up after N, no-retry on 4xx, retry on connection error, connection reuse, timeout pass-through, both run-creation backoff directions, and four pinning the POST policy — read timeout on POST is not retried (1 request) while a 503 on POST still is (3 requests), 504 on POST is not retried, and a read timeout on GET still is. Plustest_api_timeout_and_retries_reach_the_api_clientintests/test_emissions_tracker.py, asserting the two config knobs survive the constructor → config →ApiClientpath. Reverting either change fails these.They use a stdlib loopback server rather than
requests_mockon purpose:requests_mockreplaces the transport adapter, so it never executes theHTTPAdapter/Retrylayer under test — arequests_mockretry test would pass while testing nothing.One thing to flag honestly:
tests/test_emissions_tracker_flush.pyandtests/test_logging_output.pyflake on this machine (flush produces 0 or 2 rows instead of the expected 1 or 3). Verified pre-existing: it reproduces onmasterand on this branch's parent commit at the same or higher rate, and it is timing-driven, so #1339 is the PR that touches it.Not included
Aligning the direct
requests.getcalls inelectricitymaps_api/geography.pywith the same policy, and a delivery-failure counter in the final log line. Both separate concerns.🤖 Generated with Claude Code