Skip to content

Rest5Client: do not retry requests with a non-repeatable entity - #1371

Merged
l-trotta merged 1 commit into
elastic:mainfrom
hidingmyname:rest5-no-retry-non-repeatable-entity
Nov 6, 2026
Merged

l-trotta merged 1 commit into
elastic:mainfrom
hidingmyname:rest5-no-retry-non-repeatable-entity

Conversation

@hidingmyname

Copy link
Copy Markdown
Contributor

Fixes #1356

What changed

Rest5Client retried failed requests on the next node without checking whether the request entity could be sent again. A non-repeatable entity, such as an InputStreamEntity, is consumed by the first attempt, so a retry either sends an empty body and can report success (unknown length), or sends a Content-Length with no body and waits for a response (known length).

Requests with a non-repeatable entity now stop at the first failure, on all four retry paths: status codes 502/503/504 and retryable exceptions, in both performRequest and performRequestAsync. The caller gets the original ResponseException or exception. Requests with no entity or a repeatable entity are retried as before, which includes ElasticsearchClient request bodies (MultiBufferEntity is repeatable).

Trade-off

These requests also lose failover when the first attempt failed before the stream was read, for example on connection refused, which works today. The client can't tell in general how much of the stream was consumed, so it doesn't retry them at all. elasticsearch-js made the same choice in #1143, setting maxRetries to 0 for stream bodies.

Tests

The new RestClientStreamBodyRetryTests has 6 tests:

  • Mocked client with three nodes, sync and async: one attempt on a 503 and one on an IOException.
  • Two local HTTP servers, the first answering 503: the second server receives nothing.
  • Control: a repeatable entity is still tried on all three nodes.

The known-length hang has no test, because on unpatched code it would block until the test run is killed.

./gradlew :rest5-client:test
./gradlew :rest5-client:check

Results on JDK 17.0.21 (Temurin) with the Gradle wrapper (8.13):

  • Without the Rest5Client change: 118 tests, 5 failed, 1 skipped. The failures are the 5 new no-retry tests; the control passes.
  • With the change: :rest5-client:check, which includes checkstyleMain and checkstyleTest, passes with 118 tests, 0 failed, 1 skipped.

I didn't run this against a real Elasticsearch node, and I ran the checks for rest5-client only. On current main, :java-client doesn't resolve here because org.elasticsearch.client:elasticsearch-rest-client:9.6.0 isn't found, so ./gradlew check can't complete locally. This PR doesn't touch java-client.

@cla-checker-service

cla-checker-service Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

💚 CLA has been signed

@l-trotta

l-trotta commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

thanks @hidingmyname , this looks good, sorry for the broken tests, happens in prerelease days :) but it runs fine locally. could you just sign the CLA?

A non-repeatable entity, such as an InputStreamEntity, is consumed by the
first attempt. Retrying it on the next node sends an empty body when the
length is unknown, so the request can appear to succeed, and hangs when a
content length is set. Stop at the first failure for these requests, in the
synchronous and asynchronous paths.

Fixes elastic#1356
@hidingmyname
hidingmyname force-pushed the rest5-no-retry-non-repeatable-entity branch from 3f086c5 to 0b5b690 Compare November 5, 2026 17:17
@hidingmyname

Copy link
Copy Markdown
Contributor Author

Thanks! The CLA is signed and the check passes now.

@l-trotta
l-trotta merged commit 20bba9c into elastic:main Nov 6, 2026
6 of 10 checks passed
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.

Rest5Client retries requests with a non-repeatable entity, sending an empty body or hanging

2 participants