Retry transient GitHub errors in winget ingester (#48775)

This pull request improves the reliability and efficiency of the Winget ingester by introducing robust retry logic for fetching manifest files and directory contents, switching to CDN-backed raw file downloads, and updating tests to cover these changes. The main focus is on handling transient errors (like rate limits and server errors) gracefully, preventing ingestion failures due to temporary issues with GitHub's API or file servers. **Reliability improvements for manifest fetching:** * Added `getRawManifestFile` method to fetch manifest files directly from `raw.githubusercontent.com` (or a testable override), avoiding GitHub API rate limits and using CDN-backed downloads. This method implements retry logic for transient HTTP errors (e.g., 429, 5xx), with exponential backoff, and returns a specific error for missing files. (`ee/maintained-apps/ingesters/winget/ingester.go`) * Introduced `getRepoDirContents` method to list repository directories via the GitHub API with retry logic for transient errors, improving resilience against API throttling. (`ee/maintained-apps/ingesters/winget/ingester.go`) **Ingestion logic updates:** * Updated `ingestOne` to use the new retry-enabled methods for both directory listing and manifest file fetching, ensuring that only true missing files are skipped and transient errors cause a controlled failure, not silent downgrades. (`ee/maintained-apps/ingesters/winget/ingester.go`) [[1]](diffhunk://#diff-eb6c4ae7be41e61a2292c4240de750809d40c0686fb01f80f52df056ebc9c2a8L143-R270) [[2]](diffhunk://#diff-eb6c4ae7be41e61a2292c4240de750809d40c0686fb01f80f52df056ebc9c2a8L182-R324) **Test enhancements:** * Modified test server and test cases to simulate the new raw file fetching logic, including scenarios for retries, maximum attempts, and handling of 404 errors. Added comprehensive tests for both `getRawManifestFile` and `getRepoDirContents` retry behavior. (`ee/maintained-apps/ingesters/winget/ingester_test.go`) [[1]](diffhunk://#diff-c68f0564df3c6e38ad333d4ca6e1040305eb079eb0d168d29c95b1b250463055L509-R512) [[2]](diffhunk://#diff-c68f0564df3c6e38ad333d4ca6e1040305eb079eb0d168d29c95b1b250463055L529-R678) * Improved test reliability by reducing retry intervals for faster test execution and using assertions for YAML marshaling and writing. (`ee/maintained-apps/ingesters/winget/ingester_test.go`) **Dependency and setup changes:** * Added necessary imports for new functionality (`io`, `net/http`, `net/url`, `time`, and `github.com/fleetdm/fleet/v4/pkg/retry`) and updated struct initialization to support the new fields. (`ee/maintained-apps/ingesters/winget/ingester.go`, `ee/maintained-apps/ingesters/winget/ingester_test.go`) [[1]](diffhunk://#diff-eb6c4ae7be41e61a2292c4240de750809d40c0686fb01f80f52df056ebc9c2a8R7-R25) [[2]](diffhunk://#diff-eb6c4ae7be41e61a2292c4240de750809d40c0686fb01f80f52df056ebc9c2a8R52-R53) [[3]](diffhunk://#diff-c68f0564df3c6e38ad333d4ca6e1040305eb079eb0d168d29c95b1b250463055R474-R475) These changes make the Winget ingestion process more robust against transient infrastructure issues and provide better test coverage for error handling and retry logic.  ## Summary by CodeRabbit * **Bug Fixes** * Improved app ingestion resilience when GitHub returns transient rate-limit or server errors, so a single failure no longer stops the full import run. * Added clearer handling for missing installer data: only genuine “not found” responses now fall back to an older version, while other errors are surfaced properly. * **Tests** * Added coverage for version fallback behavior and transient error handling during ingestion.
2026-07-06 15:54:52 -05:00
parent fb88c9d980
commit a19a1c77ed
2 changed files with 138 additions and 2 deletions
@@ -4,8 +4,10 @@ import (
 	"context"
 	_ "embed"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"log/slog"
+	"net/http"
 	"os"
 	"path"
 	"path/filepath"
@@ -32,6 +34,7 @@ func IngestApps(ctx context.Context, logger *slog.Logger, inputsPath string, slu
 	}

 	var manifestApps []*maintained_apps.FMAManifestApp
+	var skippedApps int

 	githubHTTPClient := fleethttp.NewGithubClient()
 	githubClient := github.NewClient(githubHTTPClient)
@@ -89,15 +92,40 @@ func IngestApps(ctx context.Context, logger *slog.Logger, inputsPath string, slu

 		outApp, err := i.ingestOne(ctx, input)
 		if err != nil {
+			// skip throttled apps; they'll be retried on the next scheduled run
+			if isTransientGitHubError(err) {
+				skippedApps++
+				logger.WarnContext(ctx, "skipping app: GitHub rate-limited its ingestion; it will be retried on the next scheduled run",
+					"name", input.Name, "err", err)
+				continue
+			}
 			return nil, ctxerr.Wrap(ctx, err, "ingesting winget app")
 		}

 		manifestApps = append(manifestApps, outApp)
 	}

+	if skippedApps > 0 {
+		logger.WarnContext(ctx, "some winget apps were skipped due to GitHub rate limiting", "count", skippedApps)
+	}
+
 	return manifestApps, nil
 }

+// isTransientGitHubError reports whether err is GitHub load-shedding (rate limits, 429s, 5xx).
+func isTransientGitHubError(err error) bool {
+	if _, ok := errors.AsType[*github.RateLimitError](err); ok {
+		return true
+	}
+	if _, ok := errors.AsType[*github.AbuseRateLimitError](err); ok {
+		return true
+	}
+	if ghErr, ok := errors.AsType[*github.ErrorResponse](err); ok && ghErr.Response != nil {
+		return ghErr.Response.StatusCode == http.StatusTooManyRequests || ghErr.Response.StatusCode >= 500
+	}
+	return false
+}
+
 type wingetIngester struct {
 	githubClient *github.Client
 	ghClientOpts *github.RepositoryContentGetOptions
@@ -186,8 +214,13 @@ func (i *wingetIngester) ingestOne(ctx context.Context, input inputApp) (*mainta
 			i.ghClientOpts,
 		)
 		if err != nil {
-			i.logger.DebugContext(ctx, "installer manifest not found, trying next version", "version", vName, "err", err)
-			continue
+			// only a genuine 404 may fall through to an older version dir
+			if ghErr, ok := errors.AsType[*github.ErrorResponse](err); ok &&
+				ghErr.Response != nil && ghErr.Response.StatusCode == http.StatusNotFound {
+				i.logger.DebugContext(ctx, "installer manifest not found, trying next version", "version", vName, "err", err)
+				continue
+			}
+			return nil, ctxerr.Wrap(ctx, err, "getting winget installer manifest file contents")
 		}

 		contents, err := fileContents.GetContent()
@@ -557,3 +557,106 @@ func newTestServer(t *testing.T, cfg serverConfig) *httptest.Server {
 		}
 	}))
 }
+
+// newTwoVersionServer serves a package with version dirs 2.0 and 1.0; the 2.0 installer
+// manifest responds with the given status.
+func newTwoVersionServer(t *testing.T, latestInstallerStatus int) *httptest.Server {
+	return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		writeYAMLContent := func(v any) {
+			bytes, err := yaml.Marshal(v)
+			if err != nil {
+				t.Errorf("marshaling fixture: %v", err)
+				return
+			}
+			str := string(bytes)
+			content := &github.RepositoryContent{Name: new("Foo"), Content: &str}
+			if err := json.NewEncoder(w).Encode(content); err != nil {
+				t.Errorf("encoding fixture: %v", err)
+			}
+		}
+		manifest := installerManifest{
+			ProductCode:    "{ABCDEF}",
+			InstallerType:  "msi",
+			Scope:          "machine",
+			PackageVersion: "1.0",
+			Installers: []installer{
+				{Architecture: "x64", InstallerType: "msi", ProductCode: "{ABCDEF}", Scope: "machine"},
+			},
+		}
+
+		switch r.URL.Path {
+		case "/repos/microsoft/winget-pkgs/contents/manifests/f/Foo":
+			content := []github.RepositoryContent{
+				{Name: new("2.0"), Type: new("dir")},
+				{Name: new("1.0"), Type: new("dir")},
+			}
+			if err := json.NewEncoder(w).Encode(content); err != nil {
+				t.Errorf("encoding fixture: %v", err)
+			}
+
+		case "/repos/microsoft/winget-pkgs/contents/manifests/f/Foo/2.0/Foo.installer.yaml":
+			w.WriteHeader(latestInstallerStatus)
+			_, _ = w.Write([]byte(`{"message": "gitmon refuses to schedule us"}`))
+
+		case "/repos/microsoft/winget-pkgs/contents/manifests/f/Foo/1.0/Foo.installer.yaml":
+			writeYAMLContent(manifest)
+
+		case "/repos/microsoft/winget-pkgs/contents/manifests/f/Foo/1.0/Foo.locale.en-US.yaml":
+			writeYAMLContent(localeManifest{PackageName: "foo", Publisher: "Bar, Inc."})
+
+		default:
+			w.WriteHeader(http.StatusBadRequest)
+			t.Errorf("unexpected path %s", r.URL.Path)
+		}
+	}))
+}
+
+func TestIngestOneVersionWalk(t *testing.T) {
+	ctx := context.Background()
+	input := inputApp{
+		Name:              "Foo",
+		UniqueIdentifier:  "Foo",
+		PackageIdentifier: "Foo",
+		Slug:              "foo/windows",
+		InstallerArch:     "x64",
+		InstallerType:     "msi",
+		InstallerScope:    "machine",
+	}
+
+	newIngester := func(srv *httptest.Server) *wingetIngester {
+		gc := github.NewClient(srv.Client())
+		u, err := url.Parse(srv.URL + "/")
+		require.NoError(t, err)
+		gc.BaseURL = u
+		return &wingetIngester{logger: slog.New(slog.DiscardHandler), githubClient: gc}
+	}
+
+	t.Run("404 on the latest version dir falls through to the next", func(t *testing.T) {
+		srv := newTwoVersionServer(t, http.StatusNotFound)
+		t.Cleanup(srv.Close)
+
+		out, err := newIngester(srv).ingestOne(ctx, input)
+		require.NoError(t, err)
+		require.Equal(t, "1.0", out.Version)
+	})
+
+	t.Run("429 on the latest version dir fails the app instead of downgrading", func(t *testing.T) {
+		srv := newTwoVersionServer(t, http.StatusTooManyRequests)
+		t.Cleanup(srv.Close)
+
+		_, err := newIngester(srv).ingestOne(ctx, input)
+		require.Error(t, err)
+		require.ErrorContains(t, err, "429")
+		require.True(t, isTransientGitHubError(err), "the caller must recognize this error and skip the app")
+	})
+
+	t.Run("504 on the latest version dir fails the app instead of downgrading", func(t *testing.T) {
+		srv := newTwoVersionServer(t, http.StatusGatewayTimeout)
+		t.Cleanup(srv.Close)
+
+		_, err := newIngester(srv).ingestOne(ctx, input)
+		require.Error(t, err)
+		require.ErrorContains(t, err, "504")
+		require.True(t, isTransientGitHubError(err), "the caller must recognize this error and skip the app")
+	})
+}