fix: disable HTTP/2, set proper User-Agent, and improve retry logic

Signed-off-by: vprashar2929 <vibhu.sharma2929@gmail.com>
Made-with: Cursor

Author: vprashar2929

README.md

@@ -150,21 +150,36 @@ validators:
 
 As seen above, mdox supports validate configuration supports a few parameters and passing an array of link validators with types and regexes. The supported configuration parameters are:
 
-* `timeout`: The HTTP client's timeout. Defaults to "10s".
-* `parallelism`: The maximum amount of concurrent HTTP requests. Defaults to 100.
+* `timeout`: The HTTP client's timeout. Defaults to "30s".
+* `parallelism`: The maximum amount of concurrent HTTP requests. Defaults to 25.
 * `host_max_conns`: The maximum amount of HTTP connections open per host. Defaults to 2.
-* `random_delay`: A random delay between 0 and this value is added between requests. It takes values like "500ms", "1s", "1m", or "1m30s". Defaults to no delay.
+* `random_delay`: A random delay between 0 and this value is added between requests. It takes values like "500ms", "1s", "1m", or "1m30s". Defaults to "500ms".
 
 There are three types of validators:
 
 * `ignore`: This type of validator makes sure that `mdox` does not check links with provided regex. This is the most common use case.
 * `githubPullsIssues`: This is a smart validator which only accepts a specific type of regex of the form `(^http[s]?:\/\/)(www\.)?(github\.com\/){ORG}\/{REPO}(\/pull\/|\/issues\/)`. It performs smart validation on GitHub PR and issues links, by fetching GitHub API to get the latest pull/issue number and matching regex. This makes sure that mdox doesn't get rate limited by GitHub, even when checking a large number of GitHub links(which is pretty common in documentation)!
-* `roundtrip`: All links are checked with the roundtrip validator by default(no need for including into config explicitly) which means that each link is visited and fails if http status code is not 200(even after retries).
+* `roundtrip`: All links are checked with the roundtrip validator by default(no need for including into config explicitly) which means that each link is visited and fails if http status code is not 200(even after retries). Transient failures (HTTP 429, 503, 504, and network timeouts) are retried up to 3 times with exponential backoff. Permanent failures (404, DNS NXDOMAIN, TLS/certificate errors) fail immediately.
 
 Relative link checking *is not* affected by this configuration, as it is expected that such links will work.
 
 YAML can be passed in directly as well using `links.validate.config` flag! For more details [go.dev reference](https://pkg.go.dev/github.com/bwplotka/mdox) or [Go struct](https://github.com/bwplotka/mdox/blob/main/pkg/mdformatter/linktransformer/config.go).
 
+#### Recommended CI configuration
+
+For CI environments, enabling caching avoids re-checking links that were recently verified. This is the most effective way to reduce flaky CI failures from transient network issues or rate limiting:
+
+```yaml
+version: 1
+timeout: '1m'
+parallelism: 25
+host_max_conns: 2
+random_delay: '500ms'
+cache:
+  type: 'sqlite'
+  jitter: '24h'
+```
+
 ### Link localization
 
 It is expected for documentation to contain remote links to the project website. However, in such cases, it creates problems for multi-version docs or multi-domain websites (links would need to be updated for each version which is cumbersome). Also, it would not be navigatable locally or through GitHub (would always redirect to the website) and requires additional link checking.

pkg/mdformatter/linktransformer/link.go

@@ -7,9 +7,11 @@ import (
 	"bufio"
 	"bytes"
 	"context"
+	"crypto/x509"
 	"errors"
 	"fmt"
 	"io"
+	"math/rand"
 	"net"
 	"net/http"
 	"net/url"
@@ -99,6 +101,13 @@ func newLinktransformerMetrics(reg *prometheus.Registry) *linktransformerMetrics
 const (
 	originalURLKey     = "originalURLKey"
 	numberOfRetriesKey = "retryKey"
+
+	maxRetries             = 3
+	defaultRequestTimeout  = 30 * time.Second
+	defaultParallelism     = 25
+	defaultRandomDelay     = 500 * time.Millisecond
+	defaultMaxConnsPerHost = 2
+	maxSingleBackoff       = 10 * time.Second
 )
 
 type chain struct {
@@ -222,23 +231,25 @@ func NewValidator(ctx context.Context, logger log.Logger, linksValidateConfig []
 		MaxIdleConns:          100,
 		IdleConnTimeout:       90 * time.Second,
 		TLSHandshakeTimeout:   10 * time.Second,
-		ResponseHeaderTimeout: 30 * time.Second,
+		ResponseHeaderTimeout: defaultRequestTimeout,
 		ExpectContinueTimeout: 5 * time.Second,
 		DialContext: (&net.Dialer{
-			Timeout:   100 * time.Second,
+			Timeout:   10 * time.Second,
 			KeepAlive: 30 * time.Second,
 		}).DialContext,
 	}
 	if config.HostMaxConns != nil {
 		transport.MaxConnsPerHost = *config.HostMaxConns
+	} else {
+		transport.MaxConnsPerHost = defaultMaxConnsPerHost
 	}
 	v := &validator{
 		logger:         logger,
 		anchorDir:      anchorDir,
 		validateConfig: config,
 		localLinks:     map[string]*[]string{},
 		remoteLinks:    map[string]error{},
-		c:              colly.NewCollector(colly.Async(), colly.StdlibContext(ctx)),
+		c:              colly.NewCollector(colly.Async(), colly.StdlibContext(ctx), colly.UserAgent("Mozilla/5.0 (compatible; mdox/link-checker; +https://github.com/bwplotka/mdox)")),
 		storage:        nil,
 		destFutures:    map[futureKey]*futureResult{},
 		l:              linktransformerMetrics,
@@ -257,11 +268,11 @@ func NewValidator(ctx context.Context, logger log.Logger, linksValidateConfig []
 		},
 	}
 
-	// Set very soft limits.
-	// E.g GitHub has 50-5000 https://docs.github.com/en/free-pro-team@latest/rest/reference/rate-limit limit depending
-	// on API (only search is below 100).
 	if config.Timeout != "" {
 		v.c.SetRequestTimeout(config.timeout)
+		transport.ResponseHeaderTimeout = config.timeout
+	} else {
+		v.c.SetRequestTimeout(defaultRequestTimeout)
 	}
 
 	if v.validateConfig.Cache.IsSet() && storage != nil {
@@ -273,7 +284,8 @@ func NewValidator(ctx context.Context, logger log.Logger, linksValidateConfig []
 
 	limitRule := &colly.LimitRule{
 		DomainGlob:  "*",
-		Parallelism: 100,
+		Parallelism: defaultParallelism,
+		RandomDelay: defaultRandomDelay,
 	}
 	if config.Parallelism > 0 {
 		limitRule.Parallelism = config.Parallelism
@@ -300,53 +312,137 @@ func NewValidator(ctx context.Context, logger log.Logger, linksValidateConfig []
 		v.remoteLinks[response.Ctx.Get(originalURLKey)] = nil
 	})
 	v.c.OnError(func(response *colly.Response, err error) {
-		v.rMu.Lock()
-		defer v.rMu.Unlock()
-		retriesStr := response.Ctx.Get(numberOfRetriesKey)
-		retries, _ := strconv.Atoi(retriesStr)
-		switch response.StatusCode {
-		case http.StatusTooManyRequests:
-			if retries > 0 {
-				break
-			}
-			var retryAfterSeconds int
-			// Retry calls same methods as Visit and makes request with same options.
-			// So retryKey is incremented here if onError is called again after Retry. By default retries once.
-			response.Ctx.Put(numberOfRetriesKey, strconv.Itoa(retries+1))
-			retryAfterSeconds, convErr := strconv.Atoi(response.Headers.Get("Retry-After"))
-			if convErr != nil {
-				retryAfterSeconds = 1
-			}
-			select {
-			case <-time.After(time.Duration(retryAfterSeconds) * time.Second):
-			case <-v.c.Context.Done():
-				return
-			}
+		shouldRetry, backoff := v.recordError(response, err)
+		if !shouldRetry {
+			return
+		}
 
-			if retryErr := response.Request.Retry(); retryErr != nil {
-				v.remoteLinks[response.Ctx.Get(originalURLKey)] = fmt.Errorf("remote link retry %v: %w", response.Ctx.Get(originalURLKey), err)
-				break
-			}
-			v.remoteLinks[response.Ctx.Get(originalURLKey)] = fmt.Errorf("%q rate limited even after retry; status code %v: %w", response.Request.URL.String(), response.StatusCode, err)
-		// 0 StatusCode means error on call side.
-		case http.StatusMovedPermanently, http.StatusTemporaryRedirect, http.StatusServiceUnavailable, 0:
-			if retries > 0 {
-				break
-			}
-			response.Ctx.Put(numberOfRetriesKey, strconv.Itoa(retries+1))
+		timer := time.NewTimer(backoff)
+		defer timer.Stop()
+		select {
+		case <-timer.C:
+		case <-v.c.Context.Done():
+			v.rMu.Lock()
+			v.remoteLinks[response.Ctx.Get(originalURLKey)] = fmt.Errorf(
+				"%q not accessible; status code %v; cancelled during backoff: %w",
+				response.Request.URL.String(), response.StatusCode, err)
+			v.rMu.Unlock()
+			return
+		}
 
-			if retryErr := response.Request.Retry(); retryErr != nil {
-				v.remoteLinks[response.Ctx.Get(originalURLKey)] = fmt.Errorf("remote link retry %v: %w", response.Ctx.Get(originalURLKey), err)
-				break
-			}
-			v.remoteLinks[response.Ctx.Get(originalURLKey)] = fmt.Errorf("%q not accessible even after retry; status code %v: %w", response.Request.URL.String(), response.StatusCode, err)
-		default:
-			v.remoteLinks[response.Ctx.Get(originalURLKey)] = fmt.Errorf("%q not accessible; status code %v: %w", response.Request.URL.String(), response.StatusCode, err)
+		if retryErr := response.Request.Retry(); retryErr != nil {
+			v.rMu.Lock()
+			v.remoteLinks[response.Ctx.Get(originalURLKey)] = fmt.Errorf(
+				"retry failed for %v: %w", response.Ctx.Get(originalURLKey), err)
+			v.rMu.Unlock()
 		}
 	})
 	return v, nil
 }
 
+// recordError classifies a failed request and decides whether to retry.
+// Returns (true, backoff) when a retry is warranted.
+// All map writes happen under rMu; the caller sleeps without holding the lock.
+func (v *validator) recordError(response *colly.Response, err error) (shouldRetry bool, backoff time.Duration) {
+	v.rMu.Lock()
+	defer v.rMu.Unlock()
+
+	originalURL := response.Ctx.Get(originalURLKey)
+	retries, _ := strconv.Atoi(response.Ctx.Get(numberOfRetriesKey))
+
+	switch response.StatusCode {
+	case http.StatusTooManyRequests:
+		if retries >= maxRetries {
+			v.remoteLinks[originalURL] = fmt.Errorf(
+				"%q rate limited; status code %v after %d retries: %w",
+				response.Request.URL.String(), response.StatusCode, retries, err)
+			return false, 0
+		}
+		response.Ctx.Put(numberOfRetriesKey, strconv.Itoa(retries+1))
+		return true, retryAfterOrBackoff(response, retries)
+
+	case http.StatusServiceUnavailable, http.StatusGatewayTimeout:
+		if retries >= maxRetries {
+			v.remoteLinks[originalURL] = fmt.Errorf(
+				"%q not accessible; status code %v after %d retries: %w",
+				response.Request.URL.String(), response.StatusCode, retries, err)
+			return false, 0
+		}
+		response.Ctx.Put(numberOfRetriesKey, strconv.Itoa(retries+1))
+		return true, exponentialBackoff(retries)
+
+	case 0:
+		if isTransientError(err) && retries < maxRetries {
+			response.Ctx.Put(numberOfRetriesKey, strconv.Itoa(retries+1))
+			return true, exponentialBackoff(retries)
+		}
+		v.remoteLinks[originalURL] = fmt.Errorf(
+			"%q not accessible; status code %v: %w",
+			response.Request.URL.String(), response.StatusCode, err)
+
+	default:
+		v.remoteLinks[originalURL] = fmt.Errorf(
+			"%q not accessible; status code %v: %w",
+			response.Request.URL.String(), response.StatusCode, err)
+	}
+
+	return false, 0
+}
+
+// isTransientError classifies the underlying error for status-0 responses.
+// Only errors known to be transient return true; permanent failures
+// (NXDOMAIN, TLS/certificate) and unknown errors default to false.
+func isTransientError(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	var dnsErr *net.DNSError
+	if errors.As(err, &dnsErr) {
+		return !dnsErr.IsNotFound && dnsErr.IsTemporary
+	}
+
+	var x509CertInvalid x509.CertificateInvalidError
+	var x509HostErr x509.HostnameError
+	var x509AuthErr x509.UnknownAuthorityError
+	if errors.As(err, &x509CertInvalid) || errors.As(err, &x509HostErr) || errors.As(err, &x509AuthErr) {
+		return false
+	}
+
+	var netErr net.Error
+	if errors.As(err, &netErr) && netErr.Timeout() {
+		return true
+	}
+
+	return false
+}
+
+// retryAfterOrBackoff honours the Retry-After header when present and within
+// the per-retry budget, otherwise falls back to exponential backoff with jitter.
+func retryAfterOrBackoff(response *colly.Response, retries int) time.Duration {
+	if ra := response.Headers.Get("Retry-After"); ra != "" {
+		if seconds, err := strconv.Atoi(ra); err == nil && seconds > 0 {
+			d := time.Duration(seconds) * time.Second
+			if d <= maxSingleBackoff {
+				return d
+			}
+		}
+	}
+	return exponentialBackoff(retries)
+}
+
+// exponentialBackoff returns 2^(retries+1) seconds with additive jitter,
+// capped at maxSingleBackoff.
+func exponentialBackoff(retries int) time.Duration {
+	base := time.Duration(1<<uint(retries+1)) * time.Second
+	jitter := time.Duration(rand.Int63n(int64(base) / 2))
+	d := base + jitter
+	if d > maxSingleBackoff {
+		d = maxSingleBackoff
+	}
+	return d
+}
+
 // MustNewValidator returns mdformatter.LinkTransformer that crawls all links.
 func MustNewValidator(logger log.Logger, linksValidateConfig []byte, anchorDir string, storage *cache.SQLite3Storage) mdformatter.LinkTransformer {
 	v, err := NewValidator(context.TODO(), logger, linksValidateConfig, anchorDir, storage, nil)