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

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

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). HTTP 429 (Too Many Requests) is treated as a valid response since it proves the server is alive.
 
 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:
+
+```yaml
+version: 1
+timeout: '1m'
+parallelism: 25
+host_max_conns: 10
+random_delay: '1s'
+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

@@ -99,6 +99,11 @@ func newLinktransformerMetrics(reg *prometheus.Registry) *linktransformerMetrics
 const (
 	originalURLKey     = "originalURLKey"
 	numberOfRetriesKey = "retryKey"
+
+	maxRetries            = 3
+	defaultRequestTimeout = 30 * time.Second
+	defaultParallelism    = 25
+	defaultRandomDelay    = 500 * time.Millisecond
 )
 
 type chain struct {
@@ -218,14 +223,14 @@ func NewValidator(ctx context.Context, logger log.Logger, linksValidateConfig []
 	linktransformerMetrics := newLinktransformerMetrics(reg)
 	transport := &http.Transport{
 		Proxy:                 http.ProxyFromEnvironment,
-		ForceAttemptHTTP2:     true,
+		ForceAttemptHTTP2:     false,
 		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,
 	}
@@ -238,7 +243,7 @@ func NewValidator(ctx context.Context, logger log.Logger, linksValidateConfig []
 		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 +262,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 +278,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 +306,64 @@ 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
-			}
-
-			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))
+		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 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)
+		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
 		}
+
+		// In async mode Retry always returns nil (fires a new goroutine);
+		// the retry result arrives via a later OnScraped or OnError callback.
+		response.Request.Retry()
 	})
 	return v, nil
 }
 
+// recordError evaluates a failed request and decides whether to retry.
+// It returns (true, backoff) when a retry is warranted.
+// Map writes happen under rMu so the caller can sleep 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:
+		// A 429 proves the server is alive and the URL is routable.
+		// Retrying would only add load; treat as valid instead.
+		level.Debug(v.logger).Log("msg", "rate limited, treating as valid", "url", originalURL, "status", response.StatusCode)
+		v.remoteLinks[originalURL] = nil
+		return false, 0
+
+	case http.StatusServiceUnavailable, 0:
+		if retries < maxRetries {
+			response.Ctx.Put(numberOfRetriesKey, strconv.Itoa(retries+1))
+			v.remoteLinks[originalURL] = fmt.Errorf("%q not accessible; status code %v; retrying: %w", response.Request.URL.String(), response.StatusCode, err)
+			return true, time.Duration(1<<uint(retries+1)) * time.Second
+		}
+		v.remoteLinks[originalURL] = fmt.Errorf("%q not accessible; status code %v after %d retries: %w", response.Request.URL.String(), response.StatusCode, retries, err)
+
+	default:
+		v.remoteLinks[originalURL] = fmt.Errorf("%q not accessible; status code %v: %w", response.Request.URL.String(), response.StatusCode, err)
+	}
+
+	return false, 0
+}
+
 // 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)

pkg/mdformatter/linktransformer/link_test.go

@@ -7,11 +7,15 @@ import (
 	"context"
 	"database/sql"
 	"fmt"
+	"net/http"
+	"net/http/httptest"
 	"os"
 	"path/filepath"
 	"regexp"
 	"strings"
+	"sync/atomic"
 	"testing"
+	"time"
 
 	"github.com/bwplotka/mdox/pkg/cache"
 	"github.com/bwplotka/mdox/pkg/mdformatter"
@@ -309,7 +313,7 @@ func TestValidator_TransformDestination(t *testing.T) {
 			MustNewValidator(logger, []byte(""), anchorDir, nil),
 		))
 		testutil.NotOk(t, err)
-		testutil.Assert(t, strings.Contains(err.Error(), fmt.Sprintf("%v:1: \"https://docs.gfoogle.com/drawings/d/e/2PACX-1vTBFK_cGMbxFpYcv/pub?w=960&h=720\" not accessible even after retry; status code 0", relDirPath+filePath)))
+		testutil.Assert(t, strings.Contains(err.Error(), fmt.Sprintf("%v:1: \"https://docs.gfoogle.com/drawings/d/e/2PACX-1vTBFK_cGMbxFpYcv/pub?w=960&h=720\" not accessible; status code 0 after %d retries", relDirPath+filePath, maxRetries)))
 		testutil.Assert(t, strings.Contains(err.Error(), fmt.Sprintf("%v:1: \"https://bwplotka.dev/does-not-exists\" not accessible; status code 404: Not Found", relDirPath+filePath)))
 	})
 
@@ -483,3 +487,95 @@ func TestValidator_TransformDestination(t *testing.T) {
 		testutil.Equals(t, "sql: no rows in result set", err.Error())
 	})
 }
+
+func TestValidator_RetryBehavior(t *testing.T) {
+	tmpDir, err := os.MkdirTemp("", "test-retry")
+	testutil.Ok(t, err)
+	t.Cleanup(func() { testutil.Ok(t, os.RemoveAll(tmpDir)) })
+
+	logger := log.NewLogfmtLogger(os.Stderr)
+	anchorDir := tmpDir
+
+	t.Run("429 treated as valid", func(t *testing.T) {
+		var requestCount int32
+		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			atomic.AddInt32(&requestCount, 1)
+			w.Header().Set("Retry-After", "0")
+			w.WriteHeader(http.StatusTooManyRequests)
+		}))
+		defer srv.Close()
+
+		testFile := filepath.Join(tmpDir, "valid-429.md")
+		testutil.Ok(t, os.WriteFile(testFile, []byte("[link]("+srv.URL+")\n"), os.ModePerm))
+
+		_, err := mdformatter.IsFormatted(context.TODO(), logger, []string{testFile}, mdformatter.WithLinkTransformer(
+			MustNewValidator(logger, []byte(""), anchorDir, nil),
+		))
+		testutil.Ok(t, err)
+		testutil.Equals(t, int32(1), atomic.LoadInt32(&requestCount))
+	})
+
+	t.Run("503 recovers on retry", func(t *testing.T) {
+		var requestCount int32
+		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			count := atomic.AddInt32(&requestCount, 1)
+			if count <= 1 {
+				w.WriteHeader(http.StatusServiceUnavailable)
+				return
+			}
+			w.WriteHeader(http.StatusOK)
+		}))
+		defer srv.Close()
+
+		testFile := filepath.Join(tmpDir, "retry-503.md")
+		testutil.Ok(t, os.WriteFile(testFile, []byte("[link]("+srv.URL+")\n"), os.ModePerm))
+
+		_, err := mdformatter.IsFormatted(context.TODO(), logger, []string{testFile}, mdformatter.WithLinkTransformer(
+			MustNewValidator(logger, []byte(""), anchorDir, nil),
+		))
+		testutil.Ok(t, err)
+		testutil.Equals(t, int32(2), atomic.LoadInt32(&requestCount))
+	})
+
+	t.Run("301 is not retried", func(t *testing.T) {
+		var requestCount int32
+		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			atomic.AddInt32(&requestCount, 1)
+			w.WriteHeader(http.StatusMovedPermanently)
+		}))
+		defer srv.Close()
+
+		testFile := filepath.Join(tmpDir, "no-retry-301.md")
+		testutil.Ok(t, os.WriteFile(testFile, []byte("[link]("+srv.URL+")\n"), os.ModePerm))
+
+		_, err := mdformatter.IsFormatted(context.TODO(), logger, []string{testFile}, mdformatter.WithLinkTransformer(
+			MustNewValidator(logger, []byte(""), anchorDir, nil),
+		))
+		testutil.NotOk(t, err)
+		testutil.Assert(t, strings.Contains(err.Error(), "not accessible"))
+		testutil.Assert(t, !strings.Contains(err.Error(), "retries"))
+	})
+
+	t.Run("context cancellation stops retries", func(t *testing.T) {
+		var requestCount int32
+		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			atomic.AddInt32(&requestCount, 1)
+			w.WriteHeader(http.StatusServiceUnavailable)
+		}))
+		defer srv.Close()
+
+		ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+		defer cancel()
+
+		testFile := filepath.Join(tmpDir, "cancel-retry.md")
+		testutil.Ok(t, os.WriteFile(testFile, []byte("[link]("+srv.URL+")\n"), os.ModePerm))
+
+		v, vErr := NewValidator(ctx, logger, []byte(""), anchorDir, nil, nil)
+		testutil.Ok(t, vErr)
+
+		_, err := mdformatter.IsFormatted(ctx, logger, []string{testFile}, mdformatter.WithLinkTransformer(v))
+		testutil.NotOk(t, err)
+		testutil.Assert(t, strings.Contains(err.Error(), "cancelled during backoff") || strings.Contains(err.Error(), "not accessible"),
+			fmt.Sprintf("unexpected error: %v", err))
+	})
+}