Retry the connection if a socket error is because the server went away. - #979
Open
StephenWakely wants to merge 6 commits into
Open
Retry the connection if a socket error is because the server went away.#979StephenWakely wants to merge 6 commits into
StephenWakely wants to merge 6 commits into
Conversation
StephenWakely
added
the
changelog/Changed
…empt A single extra send attempt only rescues a packet if the reconnect happens to land in the instant the peer comes back. For UDS SOCK_DGRAM, connect() succeeds immediately against a stale-but-still-present socket path even while nothing is listening, so the real failure only surfaces at send() time - meaning the one-shot retry almost never helps during a real outage (e.g. an agent restart). Retry submitting a packet with the same exponential backoff already used for the initial UDS connect, bounded by socket_connect_timeout, so it can actually ride out a longer outage.
This comment has been minimized.
This comment has been minimized.
_socket_lock only guarded socket creation/teardown (get_socket()/ close_socket()) and, for SOCK_STREAM, the send itself - the SOCK_DGRAM send and the socket-fetch that precedes it were unprotected. A thread calling close_socket() (e.g. after its own failed send) could close the fd while another thread already held a reference to it and was mid-send, raising EBADF and dropping that packet with no chance to retry. Widen _socket_lock to cover the whole fetch-and-send in _xmit_packet_attempt, and make it an RLock since that now nests with the lock already taken inside get_socket()/close_socket().
rayz
requested changes
Aug 18, 2026
| except socket.timeout: | ||
| # dogstatsd is overflowing, drop the packets (mimics the UDP behaviour) | ||
| pass | ||
| return False |
Contributor
There was a problem hiding this comment.
This return bypasses the SOCK_STREAM cleanup below, which is why test_stream_cleanup is failing in ci.
| self.socket_connect_timeout, | ||
| ) | ||
| break | ||
| time.sleep(min(backoff, remaining)) |
Contributor
There was a problem hiding this comment.
Should we check the deadline after this sleep? Otherwise we might loop again even after expiration.
rayz
reviewed
Aug 18, 2026
| log.debug( | ||
| "Connection error submitting packet: %s, reconnecting and retrying", socket_err | ||
| ) | ||
| self.close_socket() |
Contributor
There was a problem hiding this comment.
Should self.close_socket() be called earlier before _socket_lock is released to avoid another sender using the failed socket?
rayz
reviewed
Aug 19, 2026
| # getting a fresh full socket_connect_timeout allowance each time. | ||
| connect_timeout = self.socket_connect_timeout | ||
| if retry_deadline is not None: | ||
| connect_timeout = retry_deadline - time.time() |
Contributor
There was a problem hiding this comment.
Should the timeout be calculated after acquiring the lock?
| def _xmit_packet(self, packet, is_telemetry): | ||
| # type: (str, bool) -> bool | ||
| retry_deadline = None | ||
| if self.socket_connect_timeout and self.socket_connect_timeout > 0: |
Contributor
There was a problem hiding this comment.
Do we need to gate this to be for UDS only?
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
2 participants
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.
What does this PR do?
Retries reconnecting and resending a payload on send errors socket error.
Description of the Change
On transient send errors -
errno.ECONNREFUSED,errno.ECONNRESET,errno.ENOTCONN,errno.EPIPE,errno.ENOENT, close the socket and attempt to resend. We only attempt resending once to avoid getting stuck in a loop if the agent doesn't come back beyond thesocket_connect_timeout.Alternate Designs
Possible Drawbacks
Verification Process
Additional Notes
Release Notes
Review checklist (to be filled by reviewers)
changelog/label attached. If applicable it should have thebackward-incompatiblelabel attached.do-not-merge/label attached.kind/andseverity/labels attached at least.