Skip to content

feat: retry azul apis only when retry-after value is specified (#4561) - #4993

Draft
hunterckx wants to merge 5 commits into
mainfrom
hunter/4561-retry-only-with-retry-after
Draft

hunterckx wants to merge 5 commits into
mainfrom
hunter/4561-retry-only-with-retry-after

Conversation

@hunterckx

@hunterckx hunterckx commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Re: #4561

Needs Findable update including the changes from DataBiosphere/findable-ui#1123

anvil-cmg, hca-dcp, and lungmap are given a Ky config that prevents retrying on responses that lack a value in the Retry-After header.

Note: For the sake of running it in Jest, Ky is added to the transpiled packages in the Next config

@hunterckx
hunterckx force-pushed the hunter/4561-retry-only-with-retry-after branch from 264c474 to 8f9e76d Compare October 16, 2026 00:01
NoopDog
NoopDog previously approved these changes Oct 16, 2026

@NoopDog NoopDog left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Bump findable-ui once #1123 is released. CI is red until then, because 57.0.0 doesn't have kyOptions.
  2. Check Azul prod before the prod release. As of Azul's Oct 6 comment, Access-Control-Expose-Headers is only live in the lower deployments. If prod doesn't expose Retry-After when this ships, the browser can't see the header, and prod stops retrying 429s and 503s entirely.
  3. 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 fetch reports 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.
  4. 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.
  5. 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.ts can be one isHTTPError(error) && !error.response.headers.get("Retry-After") ? false : undefined, and the empty-string Retry-After test covers a value Azul won't send.

@NoopDog
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

No deployments
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.

2 participants