feat: add specific exceptions on provider errors and add retry#103
feat: add specific exceptions on provider errors and add retry#103levivannoort wants to merge 1 commit into
Conversation
Greptile SummaryThis PR adds typed exceptions (
Confidence Score: 3/5The retry logic in Adapter.php has a sequencing defect that silently defeats retries on the most common real-world 5xx scenario. The JSON decode block runs before the HTTP status code is inspected, so a 500/503 response carrying Content-Type: application/json with a non-JSON body throws ProviderRequestFailed immediately without entering the retry path. src/VCS/Adapter.php — specifically the ordering of JSON decoding relative to the HTTP status checks inside the retry loop. Important Files Changed
Reviews (1): Last reviewed commit: "feat: add specific exceptions on provide..." | Re-trigger Greptile |
| if ($decode) { | ||
| $length = strpos($responseType, ';') ?: strlen($responseType); | ||
| switch (substr($responseType, 0, $length)) { | ||
| case 'application/json': | ||
| $json = \json_decode($responseBody, true); | ||
|
|
||
| $responseType = $responseHeaders['content-type'] ?? ''; | ||
| $responseStatus = \curl_getinfo($ch, CURLINFO_HTTP_CODE); | ||
| if ($json === null) { | ||
| throw new ProviderRequestFailed('Failed to parse response: ' . $responseBody, $responseStatus); | ||
| } | ||
|
|
||
| if ($decode) { | ||
| $length = strpos($responseType, ';') ?: strlen($responseType); | ||
| switch (substr($responseType, 0, $length)) { | ||
| case 'application/json': | ||
| $json = \json_decode($responseBody, true); | ||
| $responseBody = $json; | ||
| $json = null; | ||
| break; | ||
| } |
There was a problem hiding this comment.
JSON decode runs before HTTP status check, breaking 5xx retry
The JSON decoding block (lines 419–432) executes before the 5xx retry check (line 449). When a server returns a 500/503 with Content-Type: application/json but a non-JSON body (e.g., an HTML error page from a proxy/load balancer), json_decode returns null and ProviderRequestFailed is thrown immediately at line 426 — skipping all retry attempts for that request. The status code check for retryable errors should happen before decoding so that the retry path isn't short-circuited.
| $lastException = null; | ||
| $lastResponseStatus = 0; | ||
| $lastResponseBody = ''; | ||
| $lastResponseHeaders = []; |
There was a problem hiding this comment.
Unused variables
$lastResponseBody and $lastResponseHeaders
Both variables are assigned inside the retry loop but are never read anywhere in the method. They can be removed.
| $lastException = null; | |
| $lastResponseStatus = 0; | |
| $lastResponseBody = ''; | |
| $lastResponseHeaders = []; | |
| $lastException = null; | |
| $lastResponseStatus = 0; |
| $responseBody = \curl_exec($ch) ?: ''; | ||
|
|
||
| if ($method != self::METHOD_GET) { | ||
| curl_setopt($ch, CURLOPT_POSTFIELDS, $query); | ||
| } | ||
| if ($responseBody === true) { | ||
| $responseBody = ''; | ||
| } | ||
|
|
||
| // Allow self signed certificates | ||
| if ($this->selfSigned) { | ||
| curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, false); | ||
| curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, false); | ||
| } | ||
| $curlErrno = curl_errno($ch); |
There was a problem hiding this comment.
$responseBody === true is unreachable
curl_exec() with CURLOPT_RETURNTRANSFER=1 returns a string or false; the ?: '' operator already converts false to '', so $responseBody is always a string at this point and can never equal true. The check can be removed.
| $responseBody = \curl_exec($ch) ?: ''; | |
| if ($method != self::METHOD_GET) { | |
| curl_setopt($ch, CURLOPT_POSTFIELDS, $query); | |
| } | |
| if ($responseBody === true) { | |
| $responseBody = ''; | |
| } | |
| // Allow self signed certificates | |
| if ($this->selfSigned) { | |
| curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, false); | |
| curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, false); | |
| } | |
| $curlErrno = curl_errno($ch); | |
| $responseBody = \curl_exec($ch) ?: ''; | |
| $curlErrno = curl_errno($ch); |
| // Server errors (5xx) — retry | ||
| if ($responseStatus >= 500) { | ||
| $lastResponseStatus = $responseStatus; | ||
| $lastResponseBody = $responseBody; | ||
| $lastResponseHeaders = $responseHeaders; | ||
| if ($attempt < $this->maxRetries) { |
There was a problem hiding this comment.
Remove the two corresponding in-loop assignments of the unused variables
| // Server errors (5xx) — retry | |
| if ($responseStatus >= 500) { | |
| $lastResponseStatus = $responseStatus; | |
| $lastResponseBody = $responseBody; | |
| $lastResponseHeaders = $responseHeaders; | |
| if ($attempt < $this->maxRetries) { | |
| // Server errors (5xx) — retry | |
| if ($responseStatus >= 500) { | |
| $lastResponseStatus = $responseStatus; | |
| if ($attempt < $this->maxRetries) { |
No description provided.