Update retry handling for 503/504 errors, update tests

docs/configuration.md

@@ -194,6 +194,8 @@ $params = [
 
 ### Retries
 
+HTTP 503 and 504 responses are retried within the configured attempt limit, including empty bodies, HTML, and JSON without an `error` field. Their exception messages include the HTTP status. When attempts are exhausted, `NoMoreNodesException::getCode()` contains the last failure's HTTP status (or `0` for a connection failure), and `getPrevious()` provides the original exception. For a `ResponseException`, the status is also available through `getResponse()->getStatusCode()`.
+
 By default, the number of retries is equal to the number of defined hosts.
 
 If the number of hosts is 10 and retries are set to 5, the query will retry on 5 hosts according to the connection strategy and end with an error after 5 attempts.
@@ -218,4 +220,4 @@ A single host example:
         $params = ['host' => '123.0.0.1', 'port' => '1234', 'retries' => 2];
         $client =  new Client($params);
 ```
-<!-- proofread -->
+<!-- proofread -->

src/Manticoresearch/Client.php

@@ -443,24 +443,26 @@ public function request(Request $request, array $params = [], string $retryReaso
 			$this->initConnections();
 			throw $e;
 		} catch (ConnectionException | ResponseException $e) {
+			$e->setRequest($request);
 			if (!$this->connectionPool->retries) {
-				throw new NoMoreNodesException($e);
+				throw new NoMoreNodesException($e->getMessage(), $request, $e->getCode(), $e);
 			}
 
 			if ($e instanceof ResponseException) {
-				// We apply retrying to 502-600 responses only
+				// Retry HTTP 503 and 504 responses only.
 				if (!$e->getResponse()->is5xxRetriedStatus()) {
 					throw $e;
 				}
-				$e->setRequest($request);
 				$failContext = 'bad response';
 			} else {
 				$failContext = 'attempt';
 			}
 			$exMsg = $e->getMessage();
 			// We rely on the common error message format from Manticore here
 			$exReasonPos = strrpos($exMsg, ':');
-			$exceptionReason = substr($exMsg, ($exReasonPos === false) ? 0 : $exReasonPos + 1);
+			$exceptionReason = $e instanceof ResponseException
+				? $exMsg
+				: substr($exMsg, ($exReasonPos === false) ? 0 : $exReasonPos + 1);
 			$this->logger->warning(
 				"Manticore Search Request failed on $failContext " . $this->connectionPool->retriesAttempts . ':',
 				[
@@ -473,7 +475,14 @@ public function request(Request $request, array $params = [], string $retryReaso
 				$connection->mark(false);
 			}
 
-			return $this->request($request, $params, $exceptionReason);
+			try {
+				return $this->request($request, $params, $exceptionReason);
+			} catch (NoMoreNodesException $ex) {
+				if ($ex->getPrevious() !== null) {
+					throw $ex;
+				}
+				throw new NoMoreNodesException($ex->getMessage(), $request, $e->getCode(), $e);
+			}
 		}
 
 		return $this->lastResponse;

src/Manticoresearch/Exceptions/ConnectionException.php

@@ -24,10 +24,12 @@ class ConnectionException extends \RuntimeException implements ExceptionInterfac
 	 * ConnectionException constructor.
 	 * @param string $message
 	 * @param Request|null $request
+	 * @param int $code
+	 * @param \Throwable|null $previous
 	 */
-	public function __construct($message = '', ?Request $request = null) {
+	public function __construct($message = '', ?Request $request = null, int $code = 0, ?\Throwable $previous = null) {
 		$this->request = $request;
-		parent::__construct($message);
+		parent::__construct($message, $code, $previous);
 	}
 
 	/**

src/Manticoresearch/Exceptions/ResponseException.php

@@ -34,7 +34,13 @@ public function __construct(Request $request, Response $response) {
 		$this->request = $request;
 		$this->response = $response;
 
-		parent::__construct($response->getError());
+		$message = $response->getError();
+		$code = 0;
+		if ($response->is5xxRetriedStatus()) {
+			$code = $response->getStatusCode();
+			$message = 'HTTP ' . $code . ($message !== '' ? ': ' . $message : '');
+		}
+		parent::__construct($message, $code);
 	}
 
 	/**

src/Manticoresearch/Response.php

@@ -129,6 +129,9 @@ private function stripBadUtf8() {
 	 */
 	public function hasError() {
 		$response = $this->getResponse();
+		if ($this->is5xxRetriedStatus()) {
+			return true;
+		}
 		if (is_array($response)) {
 			foreach ($response as $r) {
 				if (isset($r['error']) && $r['error'] !== '') {
@@ -197,6 +200,10 @@ public function is5xxRetriedStatus() {
 		return $this->status >= 503 && $this->status < 505;
 	}
 
+	public function getStatusCode(): ?int {
+		return $this->status;
+	}
+
 	/**
 	 *  set request info
 	 * @param array $info

test/Manticoresearch/ClientTest.php

@@ -12,11 +12,14 @@
 use Manticoresearch\Connection;
 use Manticoresearch\Connection\Strategy\Random;
 use Manticoresearch\Exceptions\ConnectionException;
+use Manticoresearch\Exceptions\NoMoreNodesException;
+use Manticoresearch\Exceptions\ResponseException;
 use Manticoresearch\Request;
 use Manticoresearch\Response;
 use Manticoresearch\Response\Token;
 use Manticoresearch\Table;
 use Manticoresearch\Test\Helper\PopulateHelperTest;
+use Manticoresearch\Transport\TransportInterface;
 use PHPUnit\Framework\TestCase;
 
 class ClientTest extends TestCase
@@ -213,6 +216,89 @@ public function testTokenEndpointCanReturnResponseObject() {
 		$this->assertInstanceOf(Token::class, $client->token(true));
 	}
 
+	public function testRetryableResponsesRecover() {
+		foreach ([503, 504] as $status) {
+			foreach (['<html>timeout</html>', '', '{}', '{"message":"timeout"}', '{"error":"timeout"}'] as $body) {
+				$client = $this->createRetryClient(
+					[new Response($body, $status), new Response('{"ok":true}', 200)],
+					2
+				);
+				$this->assertSame(['ok' => true], $client->request($this->createRetryRequest())->getResponse());
+				$this->assertSame(0, $client->getConnectionPool()->retriesAttempts);
+			}
+		}
+	}
+
+	public function testExhaustedRetriesPreserveStatusesAndLastException() {
+		$client = $this->createRetryClient(
+			[
+				new Response('<html>unavailable</html>', 503),
+				new Response('{"error":"proxy: upstream timeout"}', 504),
+			],
+			2
+		);
+		$request = $this->createRetryRequest();
+		try {
+			$client->request($request);
+			$this->fail('Expected exhausted retries');
+		} catch (NoMoreNodesException $e) {
+			$this->assertStringContainsString('HTTP 503: Syntax error', $e->getMessage());
+			$this->assertStringContainsString('HTTP 504: "proxy: upstream timeout"', $e->getMessage());
+			$this->assertSame(504, $e->getCode());
+			$this->assertSame($request, $e->getRequest());
+			$this->assertInstanceOf(ResponseException::class, $e->getPrevious());
+			$this->assertSame(504, $e->getPrevious()->getResponse()->getStatusCode());
+			$this->assertSame($request, $e->getPrevious()->getRequest());
+		}
+	}
+
+	public function testDisabledRetriesPreserveHttpStatus() {
+		foreach ([503, 504] as $status) {
+			$client = $this->createRetryClient([new Response('{}', $status)], 0);
+			try {
+				$client->request($this->createRetryRequest());
+				$this->fail('Expected HTTP failure');
+			} catch (NoMoreNodesException $e) {
+				$this->assertSame('HTTP ' . $status, $e->getMessage());
+				$this->assertSame($status, $e->getCode());
+				$this->assertInstanceOf(ResponseException::class, $e->getPrevious());
+			}
+		}
+	}
+
+	public function testOtherHttpErrorsAreNotRetried() {
+		$client = $this->createRetryClient([new Response('{"error":"invalid query"}', 400)], 2);
+		$this->expectException(ResponseException::class);
+		$this->expectExceptionMessage('"invalid query"');
+		$client->request($this->createRetryRequest());
+	}
+
+	private function createRetryClient(array $responses, int $retries): Client {
+		$transport = $this->createMock(TransportInterface::class);
+		$transport->expects($this->exactly(sizeof($responses)))->method('execute')->willReturnCallback(
+			static function (Request $request) use (&$responses) {
+				$response = array_shift($responses);
+				if ($response->hasError()) {
+					throw new ResponseException($request, $response);
+				}
+				return $response;
+			}
+		);
+		$connection = $this->getMockBuilder(Connection::class)
+			->setConstructorArgs([['persistent' => false]])
+			->onlyMethods(['getTransportHandler'])
+			->getMock();
+		$connection->expects($this->exactly(sizeof($responses)))->method('getTransportHandler')->willReturn($transport);
+		return new Client(['connections' => [$connection], 'retries' => $retries]);
+	}
+
+	private function createRetryRequest(): Request {
+		$request = new Request(['body' => []]);
+		$request->setPath('/search');
+		$request->setMethod('GET');
+		return $request;
+	}
+
 	private function createTokenClient() {
 		return new class extends Client {
 			private $tokenRequest;

test/Manticoresearch/ResponseTest.php

@@ -47,7 +47,8 @@ public function test5xxInvalidJsonResponseException() {
 		$response = new Response($payload, 503);
 
 		$this->expectException(ResponseException::class);
-		$this->expectExceptionMessage('Syntax error');
+		$this->expectExceptionMessage('HTTP 503: Syntax error');
+		$this->expectExceptionCode(503);
 		$response->getResponse();
 	}
 
@@ -66,7 +67,19 @@ public function test504InvalidJsonWithBigintConversion() {
 		$response->enableBigintConversion();
 
 		$this->expectException(ResponseException::class);
-		$this->expectExceptionMessage('Syntax error');
+		$this->expectExceptionMessage('HTTP 504: Syntax error');
+		$this->expectExceptionCode(504);
 		$response->getResponse();
 	}
+
+	public function testRetryableStatusWithValidJson() {
+		foreach ([503, 504] as $status) {
+			foreach (['{}', '{"message":"upstream timeout"}', 'null', '"timeout"'] as $body) {
+				$response = new Response($body, $status);
+				$this->assertTrue($response->hasError());
+				$this->assertSame($status, $response->getStatusCode());
+			}
+		}
+		$this->assertFalse((new Response('{"message":"ok"}', 200))->hasError());
+	}
 }