Retries on connection failures and on a closed idle keep-alive connection

[tobixen]

I found quite some duplications - #620, #648 and #647 is closed now, and this issue is now used for tracking retry behaviour in general.

The brief summary is like this:

• There are different kinds of failure modes where one or more retries would be appropriate.
• For some failure modes (like http keepalive connection closed), the default may very well be a quick retry.
• For most failure modes, retries should be an opt-in configurable option.

The design is written up in docs/design/RETRY_AND_RESILIENCE_DESIGN.md. It should be read before implementing anything here.

The exact mechanism of how to do retries may vary between the different http-libraries. I do believe this logic belongs in the http-library and not in the caldav library. Niquests seems to come with batteries included, and is the recommended and officially supported http-library. A "best effort" should be done to support httpx2 and the old requests as well, but we should not create special logic in the caldav library for simple http-retries. This means the current logic for handling rate-limiting errors from the server may be removed. This may be a breaking change for httpx-users.

The full text below was generated by Claude Opus.

---

Symptom

A full local test run against the docker test servers errored on one Stalwart test:

ERROR tests/test_async_integration.py::TestAsyncForStalwart::test_change_attendee_status_with_email_given
  - niquests.exceptions.ConnectionError: Remote end closed connection without response

It passes when re-run on its own, so it is a race rather than a broken test.

Diagnosis

The library has no connection-error handling at all:

• no except ConnectionError anywhere under caldav/
• no max_retries, Retry, HTTPAdapter or mount anywhere either, so the
  session runs on the HTTP library's defaults - and both requests and
  niquests default to retrying nothing

"Remote end closed connection without response" on a pooled connection means the
server closed an idle keep-alive socket and the client picked that socket for the
next request; the request was never delivered.

This is not only a test-suite problem. Any long-lived client - a daemon polling
a calendar every few minutes, a desktop client left open - will outlast some
server's keep-alive timeout and get an exception on its next call, where a
transparent retry is what every other HTTP client does.

Suggested fix

Retry once when the connection was closed before the request was sent. That
case is safe regardless of method, because nothing reached the server, so a retry
cannot duplicate a write.

Points to be careful about:

• Distinguish "closed before the request was sent" from "closed mid-response".
  Only the first is unconditionally safe; retrying a PUT/POST/DELETE that
  may have been delivered is not.
• requests/niquests and the httpx family surface this differently, and the
  async client can be running any of niquests, httpx2, httpxyz or httpx - so the
  retry belongs where the exception types are already known per backend, not in a
  single blanket except.
• Cap it at one retry, and do not stack it on top of the existing rate-limit
  retry loop in BaseDAVClient.
• There is already a connection-abort workaround for
  Unable to connect :  #158 in _async_request() (a
  probe GET to detect an auth challenge); whatever is added here should not
  collide with it.

Found while investigating intermittent failures in a full local test run, during
the v3.3.0 release preparation. Not a v3.3.0 blocker - the failure is a race,
and the released behaviour is no worse than 3.2.x - so filing for 3.4.0.

[tobixen]

I just found that we had one pull request (#648) and three issues (#620, #647 and this one) covering roughly the same thing - the need for retries on connection problems. I'm now closing the other issues and leaving this one open.

I've been playing ball with Claude and come up with a document docs/design/RETRY_AND_RESILIENCE_DESIGN.md with ideas on how to go forward. The gist of it, niquests comes with "batteries included" and can do the work if configured correctly. httpx has some gaps. I feel this logic belongs in the http library layer and not in the CalDAV library - and niquests is the recommended and supported http library, so let's push this complexity to the niquests library as for now. AI-findings below.

---

⚠️ AI-generated by Claude Opus 5 via Claude Code on behalf of tobixen, from the prompt(s): "I just noticed that issue #695, pull request #648 and issue #620 is related" / "Write up the design doc. Close two of the three issues as duplicate, fix the non-closed issue to point to the design document. Close the PR if the design document differs too much. Do some research on what batteries are included in the different http libraries (httpx2, niquests)"

This issue, #620 and #648 all turned out to be the same feature seen from three angles, so there is now one design document covering all of them, committed on master. #620 and #648 are closed; this issue stays open as the tracking issue.

The design rests on reading the installed source of all five supported HTTP libraries (niquests 3.15.2 / urllib3-future 2.15.901, requests 2.34.2 / urllib3 2.7.0, httpx 0.28.1, httpx2 2.3.0, httpcore 1.0.9). The findings that shaped it:

• All five default to retrying nothing.
• niquests takes retries= directly on Session/AsyncSession, so no adapter mounting is needed, and re-exports urllib3-future's Retry as niquests.RetryConfiguration. requests needs a mounted HTTPAdapter but takes the same Retry object, so one helper serves both.
• The httpx family's retries= reaches exactly one place in httpcore, the except (ConnectError, ConnectTimeout) in HTTPConnection._connect. Connection establishment only: no read retries, no status retries, no Retry-After, no configurable backoff. httpx2 2.3.0 adds nothing here - its transport signature is byte-identical to httpx 0.28.1. Classes of failure that urllib3 handles for niquests/requests have to be handled by us, or not at all, for the httpx fallbacks.
• urllib3 classifies "Remote end closed connection without response" as a ProtocolError, hence a read error, hence subject to the allowed_methods filter - whose default omits PROPFIND and REPORT. A plain Retry(3) would leave the failure described in this issue unretried.
• HTTPConnectionPool._get_conn() already discards dropped pooled connections via is_connection_dropped(), i.e. a select() on the socket. That closes the common window but not the race, which is why this issue happens at all and why the fix is a retry rather than a liveness check.
• niquests additionally ships proactive rate limiters (LeakyBucketLimiter, TokenBucketLimiter) and HTTP/2+ keep-alive PINGs (keepalive_delay=600, keepalive_idle_window=60). The former overlaps the interval/count half of our rate-limit feature, which we currently implement in test code - possibly a wheel we could stop reinventing.

The proposal: configure (not reimplement) transport retries at session construction - retries= on the niquests session, a mounted adapter for requests, nothing for httpx - and add one small catch for the niquests lazy-gather failure, which no transport-level configuration can reach. Defaults connect=2, read=1, status retries off. No new status-retry code in caldav.

Section 4.5 goes further and asks whether the existing 429/503 handling can be deleted in favour of Retry(status_forcelist=[429, 503]). Right direction, one verified gap in the way: urllib3 2.7 caps Retry-After in parse_retry_after() via retry_after_max (default 21600), but urllib3_future 2.15.901 has no cap and no such parameter. Delegating today would therefore lose what rate_limit_max_sleep is for, and let a server answering Retry-After: 86400 park the client for a day. Porting retry_after_max upstream to urllib3-future is the cheap fix; the deletion itself is an API break and belongs in 4.0.

Six open questions are listed in section 8 of the document and want a human decision - the most consequential being whether read retries should default to 1 (fixes this issue out of the box, but a hung server is then waited on twice) or 0.

⚠️ AI-generated by Claude Opus 5 via Claude Code on behalf of tobixen.