Repository navigation
Conversation
Contributor
|
Claude finished @mbevc1's task in 1m 11s —— View job Follow-up review of commit
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
Follow-up review of commits
|
Contributor
Follow-up review of commit
|
When the retry client exhausted its retries on a 409, 429 or 5xx, its default error handler drained the last response and returned only "giving up after N attempt(s)". A lock conflict, a rate limit and a server failure were indistinguishable from the CLI output, unlike every other non-2xx path, which already reports the status. A custom ErrorHandler now appends "HTTP <status>" to the giving-up error. It also drops the method and URL the default handler repeated, because the *url.Error that http.Client wraps the error in already names them. Transport errors keep their underlying cause.
Retries used retryablehttp's default exponential backoff, so clients that failed together waited exactly as long as each other and collided again on every retry. Parallel CI jobs attesting to one trail are the common case: the v2.55.0 release failed when about half of its matrix jobs gave up at once. Each wait is now stretched by a random 0-50%, capped at the maximum wait. Stretching rather than shrinking keeps the total time before giving up at least what it was, so a server blip the CLI recovers from today is still recovered from. A wait the server sets through Retry-After is left untouched, and the number of attempts and the statuses retried are unchanged.
The handler had three return paths that each rebuilt the same message. It now builds the message once, adding the status when there is a last response and the cause when there is an error. The comments on the handler and the backoff are cut to the facts the code cannot show.
The backoff skipped the jitter whenever a response carried a Retry-After header. retryablehttp only honours that header on a 429 or 503, and only when it parses, so a 409 lock conflict from a proxy that adds the header, or a 429 with an unparseable value, got the plain exponential wait with no jitter. The backoff now treats a wait as server-set only when it differs from the plain exponential wait, which matches the library's own rules without copying them.
"HTTP 409" leaves the reader to know what the code means. The error now reads "HTTP 409 Conflict" or "HTTP 429 Too Many Requests", which tells a lock conflict from a rate limit at a glance. The text comes from http.StatusText rather than resp.Status, so the server's reason phrase never reaches the output and a hand-built response without a status line still renders.
mbevc1
force-pushed
the
20261106_fix_error_msgs
branch
from
November 6, 2026 20:53
8eeaef1 to
ede8bf2
Compare
mbevc1
enabled auto-merge (squash)
November 6, 2026 20:55
3 tasks done
* fix: report non-JSON error responses by their HTTP status An error response whose body is not JSON, such as a proxy's HTML page or an empty 401 or 404, was reported by the JSON decoder's complaint alone, e.g. "invalid character '<' looking for beginning of value". The status was carried on the APIError but never reached the message, so the user could not tell a firewall block from a missing resource. The message is now the status, such as "HTTP 403 Forbidden". When the body is not empty it adds that the response is not JSON and, unless --debug is already on, that rerunning with --debug shows it, since the body is logged at debug level only. * fix: treat a whitespace-only error body as empty Some proxies answer a 401 or 404 with a lone newline. That body counted as non-empty, so the user was told to rerun with --debug to see it, and --debug then showed nothing. The body is now trimmed before the check. Also renames the test case for an unrouted GET, which gets an empty 404 rather than the HTML body its old name described.
Contributor
Follow-up review of commit
|
3 tasks done
…1351) * refactor: export HTTPStatus for use outside the requests package Other packages report HTTP errors by bare code and are about to render them the same way as the API client does, so the helper needs to be reachable from them. * fix: name the status in upload errors An upload refused by the storage bucket read "upload refused with HTTP 403". It now reads "HTTP 403 Forbidden", like the API client's errors. * fix: name the status in kosli evaluate errors A refused policy fetch and a server-side evaluation the API turned down were reported by bare code, e.g. "the Kosli server answered 403". They now read "HTTP 403 Forbidden", like the API client's errors. * fix: name the status in Bitbucket pull request errors A pull request or commit list that came back with a success other than 200 was reported as "got HTTP status 202". It now reads "got HTTP 202 Accepted", like the API client's errors. The fake Bitbucket server in the tests can now answer a reply with a chosen status, to reach these paths. * fix: say what Bitbucket should have answered instead of blaming permissions The pull request details error reached only on a 201 or 202 told the user to review repository permissions, which a success status does not suggest. Both Bitbucket errors on that path now say the reply was not the 200 OK the code reads. The tests match the whole message. Also pins the evaluate error for a server message that is present but empty, which reports the status alone.
This branch has not been deployed
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
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.
When the shared HTTP client exhausts its retries on a 409, 429 or 5xx, the CLI reports only
giving up after N attempt(s). That makes a lock conflict, a rate limit and a server failure indistinguishable, as in the v2.55.0 release failure.ErrorHandlerappends the last response's status, e.g.Post "<url>": giving up after 4 attempt(s): HTTP 409. The method and URL that the default handler repeated are dropped, since the*url.Errorwrapper already names them. Transport errors keep their underlying cause.Retry-Afterwait is kept as is. This is a separate commit.Closes #1348
Checklist
charts/k8s-reporter/) updated, if needed. Note: these changes live in a separate PR (not needed)