Skip to content

fix: retry events when reading the HTTP response fails - #82

Open
RishavTiwari25 wants to merge 1 commit into
amplitude:mainfrom
RishavTiwari25:fix/http-client-connection-errors
Open

RishavTiwari25 wants to merge 1 commit into
amplitude:mainfrom
RishavTiwari25:fix/http-client-connection-errors

Conversation

@RishavTiwari25

@RishavTiwari25 RishavTiwari25 commented Oct 26, 2026 •

Copy link
Copy Markdown

Summary

Events are dropped silently when the HTTP response cannot be read.

urllib.request.urlopen() wraps only the errors raised while sending the request in URLError; h.getresponse() and res.read() are outside that wrapper. So HttpClient.post() raised for:

  • a connection closed or reset by the server, a load balancer or a proxy after the request was sent (http.client.RemoteDisconnected, ConnectionResetError). This is common with idle keep-alive connections and in serverless runtimes;
  • a body cut short (http.client.IncompleteRead);
  • a response body that is not the expected JSON, e.g. an HTML page from a proxy (JSONDecodeError) or JSON without code (KeyError).

The exception came out of Workers.send() inside the ThreadPoolExecutor. Nothing reads that future on the normal buffer_consumer path, so the batch was gone: no retry, no callback, no log line.

Fix: HttpClient.post() catches these (OSError, http.client.HTTPException, ValueError, KeyError) after the existing timeout/HTTPError/URLError handlers and returns a Response with HttpStatus.UNKNOWN and the error message. ResponseProcessor already retries UNKNOWN (#70), with the usual backoff and max-retry handling. Events get an insert_id from ContextPlugin, so if the server had already ingested a request whose response was lost, the retry is deduplicated.

Tests: new src/test/test_http_client.py patches urlopen: a JSON success still parses; RemoteDisconnected, ConnectionResetError and IncompleteRead, and non-JSON / missing-code bodies, return UNKNOWN with an error message. These 5 cases raise on main. Full suite (python -m unittest discover -s ./src -p 'test_*.py'): 121 tests OK.

The test module keeps its own reference to the real HttpClient.post, because some existing test modules assign a MagicMock to HttpClient.post and do not restore it.

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?: No

Note

Medium Risk
Changes error handling on the event upload path; misclassification could affect retry vs. drop behavior, but behavior aligns with existing UNKNOWN retry logic.

Overview
Fixes silent event loss when the Amplitude HTTP client fails while reading the response (after the request was already sent).

HttpClient.post() now catches OSError, HTTPException, ValueError, and KeyError—covering dropped connections (RemoteDisconnected, ConnectionResetError), truncated bodies (IncompleteRead), and unexpected payloads (HTML proxies, JSON without code)—and returns a Response with HttpStatus.UNKNOWN and an error body instead of raising into the worker thread. That status is already retried by the response processor, so batches are no longer dropped without callback or logging.

Adds test_http_client.py with mocked urlopen cases for success, connection/read failures, and malformed bodies.

Reviewed by Cursor Bugbot for commit 3fa011d. Bugbot is set up for automated code reviews on this repo. Configure here.

urlopen() wraps only errors raised while sending the request in URLError.
Errors raised while reading the response escaped HttpClient.post(): a
connection closed or reset by the server or a proxy (RemoteDisconnected,
ConnectionResetError), a cut-short body (IncompleteRead), or a body that
is not the expected JSON. The exception ended Workers.send() inside the
thread pool, where nothing observed it, so the batch was dropped without
a retry, a callback or a log line.

These are now reported as HttpStatus.UNKNOWN, which the response
processor retries (events carry an insert_id, so a retry of a request
the server did ingest is deduplicated).

This branch has not been deployed

No deployments
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