Skip to content

feat(api): reuse connections and retry transient API failures - #1340

Draft
davidberenstein1957 wants to merge 2 commits into
scaling/01-emission-timestampsfrom
scaling/02-api-client-retry-and-timeouts
Draft

feat(api): reuse connections and retry transient API failures#1340
davidberenstein1957 wants to merge 2 commits into
scaling/01-emission-timestampsfrom
scaling/02-api-client-retry-and-timeouts

Conversation

@davidberenstein1957

@davidberenstein1957 davidberenstein1957 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Part of #1338.

ApiClient._request called bare requests.get/post/patch with a hardcoded timeout=2, no Session and no retry policy, and http.py:_emit logged failures away. This adds a Session with a jittered Retry adapter, configurable timeouts, and a backoff guard around run creation.

Stacked on #1341 — this branch is rebased on scaling/01-emission-timestamps and its base is set to that branch, so #1341 merges first and this PR's diff stays limited to its own two commits. Both touched api_client.py; the conflict (the imports, and one function added next to the other) was resolved in the rebase by keeping both sides. Retarget to master once #1341 lands.

Measured against a loopback stub server (no real network)

Scenario before after
50 sequential emission POSTs 50 TCP connections, 0.085s 1 connection, 0.023s
row hits two transient 503s 1 request, 0/1 rows delivered 3 requests, 1/1 delivered
endpoint refusing connections 0.00s 1.73s
endpoint hung, emission POST 2.00s 5.0s (one read timeout, not retried)
endpoint unreachable, emission POST 2.00s 10.6s (3 connect timeouts + backoff)
endpoint hung, CLI GET path 2.00s 31.1s

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

carbonserver has no idempotency key on POST /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:

GET / PATCH POST
connection error, connect timeout retry retry
429, 502, 503 retry retry
read timeout, truncated response retry no
500, 504 retry no

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) * 3 were describing different things and neither said which. Now stated separately, for the emission path's (3.05, 5) / retries=2 budget:

  • hung endpoint (connects, never answers): ~5s. Only the read times out, and POSTs no longer retry that. Was 16.5s.
  • unreachable endpoint (every connect times out): ~11s, i.e. 3.05 * 3 plus jittered backoff.
  • full chain worst case (connect timeouts until the last attempt connects and then hangs): ~16s.

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-Timer scheduler 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

  • Tighter defaults than proposed. The proposal wanted timeout=10, retries=3 on the emission path; measured, that blocks a scheduler thread for 44s against a 15s tick. This uses (3.05, 5)/retries=2 for emissions and (3.05, 10)/retries=2 for the CLI.
  • The run-creation herd guard is mandatory, not optional. With retries, a down API means three attempts per tick per client without it. The guard lives inside ApiClient._create_run, so both callers route through one fix rather than two.
  • Spill buffer skipped, agreeing with the proposal's own recommendation: retries cover transient failures, and off-by-default disk-write code on the measurement path is code nobody exercises until it's needed and broken.

Configuration

api_timeout (default 5, read timeout in seconds) and api_retries (default 2) are documented in docs/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. Plus test_api_timeout_and_retries_reach_the_api_client in tests/test_emissions_tracker.py, asserting the two config knobs survive the constructor → config → ApiClient path. Reverting either change fails these.

They use a stdlib loopback server rather than requests_mock on purpose: requests_mock replaces the transport adapter, so it never executes the HTTPAdapter/Retry layer under test — a requests_mock retry test would pass while testing nothing.

One thing to flag honestly: tests/test_emissions_tracker_flush.py and tests/test_logging_output.py flake on this machine (flush produces 0 or 2 rows instead of the expected 1 or 3). Verified pre-existing: it reproduces on master and 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.get calls in electricitymaps_api/geography.py with the same policy, and a delivery-failure counter in the final log line. Both separate concerns.

🤖 Generated with Claude Code

@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.44444% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.51%. Comparing base (0658642) to head (863b9c3).

Files with missing lines Patch % Lines
codecarbon/core/api_client.py 96.00% 2 Missing ⚠️
codecarbon/output_methods/http.py 50.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

davidberenstein1957 and others added 2 commits August 12, 2026 19:51
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
davidberenstein1957 force-pushed the scaling/02-api-client-retry-and-timeouts branch from a53dcb9 to 863b9c3 Compare August 12, 2026 17:53
@davidberenstein1957
davidberenstein1957 changed the base branch from master to scaling/01-emission-timestamps August 12, 2026 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant