retry subdomain requests to be more resilient to flakes #10832
Conversation
🦋 Changeset detectedLatest commit: c95a0d7 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| Failed to automatically backport this PR's changes to Wrangler v3. Please manually create a PR targeting the Depending on your changes, running Notes:
|
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
| beforeEach(() => { | ||
| const level = logger.loggerLevel; | ||
| logger.loggerLevel = "debug"; | ||
| return () => (logger.loggerLevel = level); |
There was a problem hiding this comment.
Oh nice, I didn't know about that way to restore values
packages/wrangler/src/__tests__/helpers/mock-workers-subdomain.ts Outdated Show resolved Hide resolved
| abortSignal, | ||
| apiToken | ||
| ); | ||
| const { response: json, status } = await fetchInternal< |
There was a problem hiding this comment.
Side note: I really think the return type should be a cast... I mean it is but it should be explicit vs templated
There was a problem hiding this comment.
It is tricky to make it a cast as it is the response.result property that needs casting.
| success: true, | ||
| errors: [], | ||
| messages: [], | ||
| } as ResponseType, |
There was a problem hiding this comment.
No because ResponseType is an unknown type parameter at this point.
petebacondarwin force-pushed the pbd/wrangler/cope-with-subdomain-request-failure branch from 2f21c30 to 024c51d Compare 
| while (flakeCount > 0) { | ||
| flakeCount--; | ||
| handlers.unshift( | ||
| http.post( | ||
| url, | ||
| () => | ||
| HttpResponse.json( | ||
| createFetchResult(null, false, [ | ||
| { code: 10013, message: "An unknown error has occurred." }, | ||
| ]), | ||
| { status: 500 } | ||
| ), | ||
| { once: true } | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Nit: Maybe just remove the { once: true } in the original handler? We are resetting handlers after each tests anyway, so I don't see a strong reason to use once here.
There was a problem hiding this comment.
It is actually on purpose. It guarantees that the test will fail if more than one request hits this endpoint.
petebacondarwin force-pushed the pbd/wrangler/cope-with-subdomain-request-failure branch from 024c51d to dc88a7c Compare 
petebacondarwin force-pushed the pbd/wrangler/cope-with-subdomain-request-failure branch from dc88a7c to c95a0d7 Compare petebacondarwin added the skip-v3-pr petebacondarwin deleted the pbd/wrangler/cope-with-subdomain-request-failure branch 3 participants
We were seeing some flakes in CI such as https://github.com/cloudflare/workers-sdk/actions/runs/18130487486/job/51595708301?pr=10766#step:6:9893
I discussed with the D&C team and they identified that this is a problem internally but there is not a clear fix at this stage.
They confirmed that an acceptable mitigation would be to retry (a few times) the request if it fails with a 500 response.
Notable aspects of this PR:
console.log()when retrying but this seems wrong, since the user shouldn't care about this - these logs have been moved toconsole.debug().fetchRequest()helper did not provide the response status to the newAPIErrorso these were not being retried - this has now been threaded through,