Repository navigation
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
| 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.
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.
024c51d to
dc88a7c
Compare
dc88a7c to
c95a0d7
Compare
4 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,