diff --git a/cmd/graphnest-server/main.go b/cmd/graphnest-server/main.go index 5231cb98..bb4e8697 100644 --- a/cmd/graphnest-server/main.go +++ b/cmd/graphnest-server/main.go @@ -388,14 +388,14 @@ func newDurableRuntime(ctx context.Context, settings config.Config, logger *slog } _, _, _, err := store.DeleteExpiredOAuth(ctx, time.Now()) return err - }, func(error) { logger.Error("durable background refresh failed") }) + }, func(err error) { logger.Error("durable background refresh failed", "error", err) }) if err != nil { cancel() return fail(err) } reconcileRequests := make(chan int64, 64) - reconcileDone := startReconcileRequests(loopCtx, reconcileRequests, reconciler.Installation, func(error) { - logger.Error("webhook reconciliation failed") + reconcileDone := startReconcileRequests(loopCtx, reconcileRequests, reconciler.Installation, func(err error) { + logger.Error("webhook reconciliation failed", "error", err) }) var backend search.SearchBackend if settings.SearchBackend == "github" { diff --git a/internal/githubapp/models.go b/internal/githubapp/models.go index bd52c6af..7cc295dc 100644 --- a/internal/githubapp/models.go +++ b/internal/githubapp/models.go @@ -26,6 +26,7 @@ type Repository struct { HTMLURL string DefaultBranch string DefaultSHA string + ErrorCode string Private bool Archived bool Disabled bool diff --git a/internal/githubapp/reconcile.go b/internal/githubapp/reconcile.go index 04fd3805..dae65e87 100644 --- a/internal/githubapp/reconcile.go +++ b/internal/githubapp/reconcile.go @@ -2,6 +2,7 @@ package githubapp import ( "context" + "errors" "fmt" ) @@ -49,20 +50,21 @@ func (r *Reconciler) All(ctx context.Context) error { return err } upstream := make(map[int64]struct{}, len(installations)) + var errs []error for _, installation := range installations { upstream[installation.ID] = struct{}{} if err := r.reconcile(ctx, installation); err != nil { - return err + errs = append(errs, err) } } for _, installationID := range local { if _, ok := upstream[installationID]; !ok { if err := r.store.DisableInstallation(ctx, installationID, "deleted"); err != nil { - return err + errs = append(errs, err) } } } - return nil + return errors.Join(errs...) } func (r *Reconciler) reconcile(ctx context.Context, installation Installation) error { @@ -73,6 +75,7 @@ func (r *Reconciler) reconcile(ctx context.Context, installation Installation) e if err != nil { return err } + var shaErrs []error for index := range repositories { repository := &repositories[index] if repository.Archived || repository.Disabled { @@ -80,8 +83,30 @@ func (r *Reconciler) reconcile(ctx context.Context, installation Installation) e } repository.DefaultSHA, err = r.github.DefaultBranchSHA(ctx, installation.ID, repository.Owner, repository.Name, repository.DefaultBranch) if err != nil { - return fmt.Errorf("read %s default branch: %w", repository.FullName, err) + repository.DefaultSHA = "" + repository.ErrorCode = "default_branch" + name := repository.FullName + if name == "" { + name = repository.Owner + "/" + repository.Name + } + if status := httpStatus(err); status != 0 { + shaErrs = append(shaErrs, fmt.Errorf("installation %d repository %s HTTP %d: read default branch: %w", installation.ID, name, status, err)) + } else { + shaErrs = append(shaErrs, fmt.Errorf("installation %d repository %s: read default branch: %w", installation.ID, name, err)) + } + continue } } - return r.store.ReconcileInstallation(ctx, installation, repositories) + if err := r.store.ReconcileInstallation(ctx, installation, repositories); err != nil { + return err + } + return errors.Join(shaErrs...) +} + +func httpStatus(err error) int { + var status HTTPStatusError + if errors.As(err, &status) { + return status.StatusCode + } + return 0 } diff --git a/internal/githubapp/reconcile_test.go b/internal/githubapp/reconcile_test.go index 881fb1b9..baceb2a3 100644 --- a/internal/githubapp/reconcile_test.go +++ b/internal/githubapp/reconcile_test.go @@ -6,6 +6,7 @@ import ( "net/http" "net/http/httptest" "reflect" + "strings" "testing" "time" ) @@ -14,6 +15,7 @@ type reconcileAPIStub struct { installations []Installation repositories map[int64][]Repository shas map[int64]string + shaErrs map[int64]error reads int } @@ -22,12 +24,17 @@ func (api *reconcileAPIStub) Installations(context.Context) ([]Installation, err } func (api *reconcileAPIStub) InstallationRepositories(_ context.Context, installationID int64) ([]Repository, error) { + api.reads = 0 return api.repositories[installationID], nil } func (api *reconcileAPIStub) DefaultBranchSHA(_ context.Context, _ int64, _ string, name, _ string) (string, error) { api.reads++ - sha, ok := api.shas[repositoryID(name)] + id := repositoryID(name) + if err, ok := api.shaErrs[id]; ok { + return "", err + } + sha, ok := api.shas[id] if !ok { return "", fmt.Errorf("missing SHA for %s", name) } @@ -58,7 +65,16 @@ func (store *reconcileStoreStub) ReconcileInstallation(_ context.Context, instal return fmt.Errorf("store called after %d of %d SHA reads", store.api.reads, expectedReads) } for _, repository := range repositories { - if !repository.Archived && !repository.Disabled && repository.DefaultSHA != store.api.shas[repository.ID] { + if repository.Archived || repository.Disabled { + continue + } + if repository.ErrorCode != "" { + if repository.DefaultSHA != "" { + return fmt.Errorf("repository %d SHA = %q with error %q", repository.ID, repository.DefaultSHA, repository.ErrorCode) + } + continue + } + if repository.DefaultSHA != store.api.shas[repository.ID] { return fmt.Errorf("repository %d SHA = %q", repository.ID, repository.DefaultSHA) } } @@ -166,11 +182,61 @@ func TestReconcileAllDisablesInstallationsMissingUpstream(t *testing.T) { } } +func TestReconcileAllContinuesAfterDefaultBranch404(t *testing.T) { + api := &reconcileAPIStub{ + installations: []Installation{ + {ID: 4121, AccountLogin: "acme", Status: "active"}, + {ID: 7, AccountLogin: "other", Status: "active"}, + }, + repositories: map[int64][]Repository{ + 4121: { + {ID: 101, InstallationID: 4121, Owner: "acme", Name: "one", FullName: "acme/one", DefaultBranch: "main"}, + {ID: 102, InstallationID: 4121, Owner: "acme", Name: "two", FullName: "acme/two", DefaultBranch: "trunk"}, + }, + 7: { + {ID: 103, InstallationID: 7, Owner: "other", Name: "three", FullName: "other/three", DefaultBranch: "main"}, + }, + }, + shas: map[int64]string{102: sha('b'), 103: sha('c')}, + shaErrs: map[int64]error{ + 101: HTTPStatusError{StatusCode: http.StatusNotFound}, + }, + } + store := &reconcileStoreStub{api: api} + err := (&Reconciler{github: api, store: store}).All(t.Context()) + if err == nil { + t.Fatal("expected default-branch error") + } + message := err.Error() + if !strings.Contains(message, "installation 4121") || !strings.Contains(message, "acme/one") || !strings.Contains(message, "HTTP 404") { + t.Fatalf("error %q missing installation, repository, or status", message) + } + if !reflect.DeepEqual(store.reconciled, []int64{4121, 7}) { + t.Fatalf("reconciled = %v", store.reconciled) + } + if len(store.repositories) != 2 { + t.Fatalf("installations stored = %d", len(store.repositories)) + } + first := store.repositories[0] + if len(first) != 2 || first[0].ErrorCode != "default_branch" || first[0].DefaultSHA != "" || first[1].DefaultSHA != sha('b') { + t.Fatalf("installation 4121 repositories = %#v", first) + } + if len(store.repositories[1]) != 1 || store.repositories[1][0].DefaultSHA != sha('c') { + t.Fatalf("installation 7 repositories = %#v", store.repositories[1]) + } +} + func repositoryID(name string) int64 { - if name == "one" { + switch name { + case "one": return 101 + case "two": + return 102 + case "three": + return 103 + default: + return 0 } - return 102 } func sha(character byte) string { diff --git a/internal/postgres/repository.go b/internal/postgres/repository.go index 02bb7f63..5fc65d82 100644 --- a/internal/postgres/repository.go +++ b/internal/postgres/repository.go @@ -276,9 +276,10 @@ func (s *Store) ReconcileInstallation(ctx context.Context, installation githubap var status string if err := tx.QueryRow(ctx, ` insert into repositories (github_id, installation_id, owner, name, clone_url, web_url, size_bytes, - default_branch, private, archived, enabled, status) + default_branch, private, archived, enabled, status, error_code) select $1, id, $3, $4, $5, $6, $7, $8, $9, $10, $11, - case when $11 then 'pending' else 'disabled' end + case when $11 then 'pending' else 'disabled' end, + nullif($12, '') from installations where github_id=$2 on conflict (github_id) do update set installation_id=excluded.installation_id, owner=excluded.owner, name=excluded.name, clone_url=excluded.clone_url, @@ -287,15 +288,18 @@ func (s *Store) ReconcileInstallation(ctx context.Context, installation githubap status=case when not excluded.enabled then 'disabled' when repositories.status='disabled' and repositories.indexed_sha=repositories.desired_sha then 'ready' when repositories.status='disabled' then 'pending' else repositories.status end, - error_code=case when excluded.enabled and repositories.status='disabled' then null - when not excluded.enabled then null else repositories.error_code end, + error_code=case when $12 <> '' then $12 + when excluded.enabled and repositories.status='disabled' then null + when not excluded.enabled then null + when repositories.error_code = 'default_branch' then null + else repositories.error_code end, updated_at=now() returning id, desired_sha, status`, repository.ID, installation.ID, repository.Owner, repository.Name, repository.CloneURL, repository.HTMLURL, repository.SizeBytes, repository.DefaultBranch, - repository.Private, repository.Archived, enabled).Scan(&id, &desiredSHA, &status); err != nil { + repository.Private, repository.Archived, enabled, repository.ErrorCode).Scan(&id, &desiredSHA, &status); err != nil { return err } - if enabled && (desiredSHA == nil || *desiredSHA != repository.DefaultSHA || status == "pending") { + if enabled && repository.ErrorCode == "" && repository.DefaultSHA != "" && (desiredSHA == nil || *desiredSHA != repository.DefaultSHA || status == "pending") { if err := enqueueIndex(ctx, tx, IndexRequest{RepositoryID: id, TargetSHA: repository.DefaultSHA}); err != nil { return err }