Skip to content

Fix user list error handling. #127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ggreer merged 1 commit into from
Mar 5, 2026
Merged

Fix user list error handling. #127

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 5 additions & 10 deletions pkg/connector/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,17 @@ package connector
import (
"context"
"fmt"
"net/http"
"net/mail"
"strconv"
"strings"

v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2"
"github.com/conductorone/baton-sdk/pkg/annotations"
"github.com/conductorone/baton-sdk/pkg/types/resource"
"github.com/conductorone/baton-sdk/pkg/uhttp"
"github.com/google/go-github/v69/github"
"github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap"
"github.com/shurcooL/githubv4"
"go.uber.org/zap"
"google.golang.org/grpc/codes"
"google.golang.org/protobuf/types/known/timestamppb"
)

Expand Down Expand Up @@ -155,15 +152,13 @@ func (o *userResourceType) List(ctx context.Context, parentID *v2.ResourceId, op
for _, user := range users {
u, res, err := o.client.Users.GetByID(ctx, user.GetID())
if err != nil {
if isRatelimited(res) {
return nil, nil, uhttp.WrapErrors(codes.Unavailable, "too many requests", err)
}
// This undocumented API can return 404 for some users. If this fails it means we won't get some of their details like email
if res == nil || res.StatusCode != http.StatusNotFound {
return nil, nil, err
if isNotFoundError(res) {
l.Warn("error fetching user by id", zap.Error(err), zap.Int64("user_id", user.GetID()))
u = user
} else {
return nil, nil, wrapGitHubError(err, res, "github-connector: failed to get user by id")
Comment on lines +156 to +160
Copy link

@coderabbitai coderabbitai bot Mar 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Treat transient GetByID errors as recoverable too

At Line 160, non-404 errors still abort the sync. That means transient GitHub failures (e.g., 502/503/504) can still restart the operation, which conflicts with this PR’s objective. Consider falling back to u = user for transient HTTP errors as well, and only returning on non-recoverable failures.

Proposed fix 
 		u, res, err := o.client.Users.GetByID(ctx, user.GetID())
 		if err != nil {
 			// This undocumented API can return 404 for some users. If this fails it means we won't get some of their details like email
-			if isNotFoundError(res) {
+			if isNotFoundError(res) || isTransientGitHubError(res) {
 				l.Warn("error fetching user by id", zap.Error(err), zap.Int64("user_id", user.GetID()))
 				u = user
 			} else {
 				return nil, nil, wrapGitHubError(err, res, "github-connector: failed to get user by id")
 			}
 		}
func isTransientGitHubError(resp *github.Response) bool {
	if resp == nil {
		return false
	}
	switch resp.StatusCode {
	case 502, 503, 504:
		return true
	default:
		return false
	}
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/user.go` around lines 156 - 160, The code currently returns an
error for any non-404 response in the GetByID handling (calling
wrapGitHubError), which aborts sync on transient GitHub failures; add a helper
isTransientGitHubError(resp *github.Response) that returns true for 502, 503,
504 and, in the block where isNotFoundError(res) is checked, treat transient
responses the same as not-found by assigning u = user instead of returning; only
call wrapGitHubError and return for truly non-recoverable responses. Ensure you
reference the existing isNotFoundError(res) and wrapGitHubError(err, res, "...")
symbols when making the change.

Copy link
Contributor Author

@ggreer ggreer Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrapGitHubError() returns codes.Unavailable for 502/503/etc, which causes syncer to retry the List() call.

Copy link

@coderabbitai coderabbitai bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ggreer, thanks for the clarification! That makes sense — since wrapGitHubError() maps 502/503/504 responses to codes.Unavailable, the syncer will automatically retry the List() call rather than treating it as a fatal failure or restarting the entire sync from scratch. The current approach is correct and the concern I raised doesn't apply here.

✏️ Learnings added 
Learnt from: ggreer
Repo: ConductorOne/baton-github PR: 127
File: pkg/connector/user.go:156-160
Timestamp: 2026-03-05T21:28:54.578Z
Learning: In the `baton-github` connector (`pkg/connector/user.go`), `wrapGitHubError()` returns `codes.Unavailable` for transient HTTP errors (502, 503, 504 etc.), which causes the syncer to retry the `List()` call rather than aborting or restarting the full sync. This means returning `wrapGitHubError(err, res, ...)` for non-404 errors in `GetByID` is intentional and correct — transient errors are already handled via the retry mechanism.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

}
l.Error("error fetching user by id", zap.Error(err), zap.Int64("user_id", user.GetID()))
u = user
}
userEmail := u.GetEmail()
var extraEmails []string
Expand Down
Loading