[fetch-utils] use default agent with a 1s timeout and 3 attempts in connect phase (#35406)

Co-authored-by: Andrew Rumble <andrew.rumble@overleaf.com>
GitOrigin-RevId: cb1e854d61ce623fa557ae003dc7159b096e1f86

libraries/fetch-utils/index.ts

@@ -1,10 +1,12 @@
 import _ from 'lodash'
-import { Readable } from 'node:stream'
+import { Readable, Duplex } from 'node:stream'
 import OError from '@overleaf/o-error'
 import fetch from 'node-fetch'
 import type { Response } from 'node-fetch'
 import http from 'node:http'
 import https from 'node:https'
+import net from 'node:net'
+import tls from 'node:tls'
 
 let logger: { warn: (...args: any[]) => void } | undefined
 
@@ -160,6 +162,13 @@ class RequestFailedError extends OError {
   }
 }
 
+function setupDefaultAgent(fetchOps: any) {
+  // Provide a function to get the agent for each request as there may be
+  // multiple requests with different protocols due to redirects.
+  fetchOps.agent = (url: URL) =>
+    url.protocol === 'https:' ? httpsAgent : httpAgent
+}
+
 function parseOpts(opts: any, url: string | URL) {
   const fetchOpts = _.omit(opts, ['json', 'signal', 'basicAuth'])
   if (opts.json) {
@@ -168,6 +177,9 @@ function parseOpts(opts: any, url: string | URL) {
   if (opts.basicAuth) {
     setupBasicAuth(fetchOpts, opts.basicAuth)
   }
+  if (!fetchOpts.agent) {
+    setupDefaultAgent(fetchOpts)
+  }
 
   const abortController = new AbortController()
   fetchOpts.signal = abortController.signal
@@ -302,34 +314,89 @@ class ConnectTimeoutError extends OError {
   }
 }
 
-function withTimeout(createConnection: any, options: any, callback: any) {
-  if (options.connectTimeout) {
-    // Wrap createConnection in a timeout
-    const timer = setTimeout(() => {
-      socket.destroy(new ConnectTimeoutError(options))
-    }, options.connectTimeout)
-    const socket = createConnection(options, (err: any, stream: any) => {
-      clearTimeout(timer)
-      callback(err, stream)
-    })
-    return socket
-  } else {
-    // Fallback to default createConnection
-    return createConnection(options, callback)
+function tryToCreateConnection(
+  createConnection: (options: any) => Duplex,
+  options: any,
+  callback: (err: any, socket: any) => void
+) {
+  let socket: Duplex
+  try {
+    socket = createConnection(options)
+  } catch (err) {
+    callback(err, null)
+    return
   }
+  const timer = setTimeout(() => {
+    socket.destroy(new ConnectTimeoutError(options))
+  }, options.connectTimeout)
+  const onConnect = () => {
+    clearTimeout(timer)
+    socket.off('error', onError)
+    callback(null, socket)
+  }
+  const onError = (err: any) => {
+    clearTimeout(timer)
+    socket.off('connect', onConnect)
+    callback(err, null)
+  }
+  socket.once('connect', onConnect)
+  socket.once('error', onError)
+}
+
+function withTimeout(
+  createConnection: (options: any) => Duplex,
+  options: any,
+  callback: (err: any, socket: any) => void
+) {
+  const attempt = (remainingAttempts: number) => {
+    remainingAttempts--
+    tryToCreateConnection(createConnection, options, (err, socket) => {
+      if (err && remainingAttempts > 0) {
+        setTimeout(() => {
+          attempt(remainingAttempts)
+        }, options.connectRetryInterval ?? 100)
+        return
+      }
+      callback(err, socket)
+    })
+  }
+  attempt(3)
 }
 
 class CustomHttpAgent extends http.Agent {
+  constructor(options: any) {
+    if (!(options.connectTimeout > 0)) {
+      throw new Error(
+        'CustomHttpAgent must be called with positive connectTimeout'
+      )
+    }
+    super(options)
+  }
   createConnection(options: any, callback: any) {
-    return withTimeout(super.createConnection.bind(this), options, callback)
+    withTimeout(net.createConnection, options, callback)
+    return undefined
   }
 }
+
 class CustomHttpsAgent extends https.Agent {
+  constructor(options: any) {
+    if (!(options.connectTimeout > 0)) {
+      throw new Error(
+        'CustomHttpsAgent must be called with positive connectTimeout'
+      )
+    }
+    super(options)
+  }
   createConnection(options: any, callback: any) {
-    return withTimeout(super.createConnection.bind(this), options, callback)
+    withTimeout(tls.connect, options, callback)
+    return undefined
   }
 }
 
+const MAX_CONNECT_TIME = 1000
+const httpAgent = new CustomHttpAgent({ connectTimeout: MAX_CONNECT_TIME })
+const httpsAgent = new CustomHttpsAgent({ connectTimeout: MAX_CONNECT_TIME })
+
 export {
   fetchJson,
   fetchJsonWithResponse,

libraries/fetch-utils/test/unit/FetchUtils.test.js

@@ -1,5 +1,6 @@
 import { expect } from 'chai'
 import dns from 'node:dns'
+import http from 'node:http'
 import { once } from 'node:events'
 import events from 'node:events'
 import fs from 'node:fs'
@@ -364,6 +365,66 @@ describe('fetch-utils', function () {
           'request to http://10.255.255.255/ failed, reason: connect timeout'
         )
     })
+
+    it('retries after a delay', { timeout: 5_000 }, async function () {
+      // The test should finish after 1.5s.
+      // t   0ms: first connect fails with ECONNREFUSED
+      // t 500ms: listen on port
+      // t1000ms: retry to connect again, works
+      // t1500ms: respond to request
+      const agent = new CustomHttpAgent({
+        connectTimeout: 100,
+        connectRetryInterval: 1_000,
+      })
+      const server = http.createServer((req, res) => {
+        setTimeout(() => {
+          res.write('hello')
+          res.end()
+          server.close()
+        }, 500)
+      })
+      // Grab a dynamic port, then close again so that the first connect
+      // attempt is refused before the server starts listening for real.
+      await once(server.listen(0), 'listening')
+      const { port } = server.address()
+      await new Promise(resolve => server.close(resolve))
+      const t0 = performance.now()
+      setTimeout(() => {
+        server.listen(port)
+      }, 500)
+      const body = await fetchString(`http://127.0.0.1:${port}`, { agent })
+      expect(body).to.equal('hello')
+      const t1 = performance.now()
+      expect(t1 - t0).to.be.at.least(1_500)
+    })
+
+    it('does not open a stray connection when the socket errors after connect', async function () {
+      const agent = new CustomHttpAgent({
+        connectTimeout: 100,
+        connectRetryInterval: 10,
+      })
+      const connections = []
+      const server = http.createServer(req => {
+        // Reset the established connection before responding. Doing this in
+        // the request handler guarantees the client finished the connect
+        // phase, so the ECONNRESET arrives after 'connect' has fired.
+        req.socket.resetAndDestroy()
+      })
+      server.on('connection', socket => connections.push(socket))
+      await once(server.listen(0), 'listening')
+      const { port } = server.address()
+      try {
+        await expect(
+          fetchString(`http://127.0.0.1:${port}/`, { agent })
+        ).to.be.rejectedWith(FetchError)
+        // Leave time for a buggy connect retry to open a stray connection.
+        await wait(200)
+        expect(connections).to.have.length(1)
+      } finally {
+        for (const socket of connections) socket.destroy()
+        server.close()
+      }
+    })
   })
 
   describe('CustomHttpsAgent', function () {