Repository navigation
Conversation
…t in ignore paths (#4561)
hunterckx
force-pushed
the
hunter/4561-retry-only-with-retry-after
branch
from
October 16, 2026 00:01
264c474 to
8f9e76d
Compare
NoopDog
previously approved these changes
Oct 16, 2026
NoopDog
left a comment
Collaborator
There was a problem hiding this comment.
Thanks Hunter, this matches what Azul asked for in #4561: only retry Azul requests whose response carries a Retry-After header. Errors without it, including 400, 500, 502 and 504, now fail straight away, and requests with it wait the header's delay and retry up to 3 times. Putting it behind kyOptions, so only the Azul sites change, is nice and tidy, and the new tests fail against the old behaviour, so they're doing their job.
Approving the policy. A few things before this ships:
- Bump findable-ui once #1123 is released. CI is red until then, because 57.0.0 doesn't have
kyOptions. - Check Azul prod before the prod release. As of Azul's Oct 6 comment,
Access-Control-Expose-Headersis only live in the lower deployments. If prod doesn't exposeRetry-Afterwhen this ships, the browser can't see the header, and prod stops retrying 429s and 503s entirely. - One question for Azul: do API Gateway's own 502 and 504 responses include CORS headers? Those errors come from the Gateway, not Azul's code (504 timeouts, 502 "payload too large"). If they arrive without CORS headers, the browser hides them and
fetchreports a plain network error, which Ky still retries (1s/3s/9s). That would retry exactly the 502s and 504s Hannes said not to. If they do have CORS headers, this PR is right as it is. If not, the cleaner fix is on Azul's side. - 307 with
Retry-After. The browser follows a 307 immediately and doesn't wait for the header's delay. Azul's #7452 was going to test this against a local Data Browser, so it's worth confirming that immediate follow-up is fine with them. - Small: the PR body says "Re: #4561". If this finishes the issue, "Closes #4561" will close it on merge. Optionally, the two checks in
constants.tscan be oneisHTTPError(error) && !error.response.headers.get("Retry-After") ? false : undefined, and the empty-stringRetry-Aftertest covers a value Azul won't send.
NoopDog
dismissed
their stale review
October 16, 2026 02:49
Dismissing my approval until the findable-ui bump lands, since this repo doesn't require green checks to merge. The feedback above still stands, and I'll re-review after the bump.
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.
Re: #4561
Needs Findable update including the changes from DataBiosphere/findable-ui#1123
anvil-cmg,hca-dcp, andlungmapare given a Ky config that prevents retrying on responses that lack a value in theRetry-Afterheader.Note: For the sake of running it in Jest, Ky is added to the transpiled packages in the Next config