Retry request on ETIMEDOUT

[kaganjd]

- Do you want to request a feature or report a bug?
feature? enhancement?

- What is the current behavior?
We have an enterprise customer who's hitting errors like this (redacted for privacy) a few times every day, impacting 5-10% of their daily builds:

FetchError: request to https://api.netlify.com/api/v1/sites/xxxxxxx-xxxxxxx failed, reason:
    connect ETIMEDOUT 3x.xxx.xx.xxx:443
    Code: ETIMEDOUT

@kitop investigated and found that the error was coming from node-fetch. He says:

Seems like that's happening because one of the requests from the client fails to connect to the API servers when polling for the deploy being ready. Some related issue: node-fetch/node-fetch#817
If I turn on my wifi and try to run a command from CLI it shows a similar error

Request
Can we retry when there's a timeout like this? It looks like we currently retry when hitting a rate limit but not when there's a network issue:

js-client/src/methods/index.js

Lines 69 to 88 in a6476c9

	const makeRequestOrRetry = async function({ url, method, NetlifyApi, requestParams, opts }) {
	for (let index = 0; index <= MAX_RETRY; index++) {
	const optsA = getOpts(method, NetlifyApi, requestParams, opts)
	const response = await makeRequest(url, optsA)
	if (shouldRetry(response, index)) {
	await waitForRetry(response)
	} else {
	return response
	}
	}
	}
	const makeRequest = async function(url, opts) {
	try {
	return await fetch(url, opts)
	} catch (error) {
	throw getFetchError(error, url, opts)
	}
	}

Additional context

• https://github.com/netlify/bitballoon/issues/7559#issuecomment-693587240 (internal)

[ehmicky]

👍

We should use got, which has built-in support for automatic retries.

[mheffner]

@erquhart Do you think Pod B could prioritize this work? A customer has been hitting it recently (https://github.com/netlify/bitballoon/issues/7520#issuecomment-704424483) failing their deploys.

[erezrokah]

Other than adding edge handlers deploy to the CLI (which should not be related), the other deploy related CLI change is netlify/cli#1272, which might have the impact of deploying more files.

Not sure how that might be related to polling to when the deploy is ready, which should be retried regardless.

Also related #157

[erquhart]

@erezrokah @ehmicky let's hit this first thing in the morning.

[ehmicky]

The deploy is failing here:

js-client/src/deploy/util.js

Lines 73 to 106 in 00b3e93

	async function waitForDeploy(api, deployId, siteId, timeout) {
	let deploy // capture ready deploy during poll
	await pWaitFor(loadDeploy, {
	interval: 1000,
	timeout,
	message: 'Timeout while waiting for deploy'
	})
	return deploy
	async function loadDeploy() {
	const d = await api.getSiteDeploy({ siteId, deployId })
	switch (d.state) {
	// https://github.com/netlify/bitballoon/blob/master/app/models/deploy.rb#L21-L33
	case 'error': {
	const deployError = new Error(`Deploy ${deployId} had an error`)
	deployError.deploy = d
	throw deployError
	}
	case 'ready': {
	deploy = d
	return true
	}
	case 'preparing':
	case 'prepared':
	case 'uploaded':
	case 'uploading':
	default: {
	return false
	}
	}
	}
	}

This is part of this library's api.deploy() method. This happens when polling GET /sites/{site_id}/deploys/{deploy_id} once per minute until the response returns { "state": "ready" }. This polling happens right after uploading files and printing the log line Waiting for deploy to go live....

This polling times out after 20 minutes (see configurable option deployTimeout). However, I do not think this 20 minutes timeout is what's causing the ETIMEDOUT above. Instead, it would create a TimeoutError (see code here and there), which is different from ETIMEDOUT. ETIMEDOUT is thrown from the Node.js core http module instead and indicates a timeout at the TCP or HTTP level:

ETIMEDOUT (Operation timed out): A connect or send request failed because the connected party did not properly respond after a period of time. Usually encountered by http or net. Often a sign that a socket.end() was not properly called.

This is probably not a bug with node-fetch either. The GitHub issue in the node-fetch repository linked above has been answered by one of the maintainers, and it does not look like a bug on their part. In the lines of code at https://github.com/node-fetch/node-fetch/blob/eaff0094c4dfdd5b78711a8c4f1b61e33d282072/src/index.js#L93-L96, node-fetch is only forwarding the error from Node.js core http module.

My most likely guess is that when performing GET /sites/{site_id}/deploys/{deploy_id}, there is a server timeout, either for the TCP connection to start, or for the HTTP headers and/or body to be received.

Node.js recently changed the HTTP timeout logic in 14.11.0 on the 15th of September. This might be unrelated though, I am just mentioning in case we starting experiencing issues on that day.

Retrying on ETIMEDOUT might patch this problem, but it looks like the core problem might be different: either something wrong with the uploading logic of this library api.deploy() method, or with our API timing out on GET /sites/{site_id}/deploys/{deploy_id}.

[erezrokah]

Could this also happen on an unstable network connection? If so retrying makes regardless of the root cause.

[kschlarman]

My assumption is that if this were an issue with our endpoint flaking out (GET /sites/{site_id}/deploys/{deploy_id}), we'd have many more reports of this issue. So my hunch is that it might be an unstable network connection.

I think it's pretty reasonable to expect that our customers may have flaky network connections. For this reason, many client libraries I've worked with will retry timeouts or other connection errors. So I view this as an awesome new feature that will improve the robustness of our CLI deploys.