Skip to content

increase timeout and retry #37

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

Open
jingglypuff wants to merge 1 commit into
base: main
Choose a base branch
from
Open

increase timeout and retry #37

Show file tree
Hide file tree
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
3 changes: 3 additions & 0 deletions pkg/connector/connector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import (
"context"
"fmt"
"time"

v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2"
"github.com/conductorone/baton-sdk/pkg/annotations"
Expand Down Expand Up @@ -120,6 +121,8 @@
if err != nil {
return nil, err
}

Check failure on line 124 in pkg/connector/connector.go

View workflow job for this annotation

GitHub Actions / go-lint 

File is not properly formatted (goimports)
httpClient.Timeout = 10 * time.Minute

Comment on lines +124 to 126
Copy link

@coderabbitai coderabbitai bot Sep 26, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fix gofmt/goimports formatting before merging.

CI failed because this file is not gofmt/goimports formatted. Please run goimports -w pkg/connector/connector.go (or gofmt) and re-push so the pipeline passes.

🧰 Tools
🪛 GitHub Check: go-lint

[failure] 124-124:
File is not properly formatted (goimports)

🪛 GitHub Actions: ci

[error] 124-124: golangci-lint: File is not properly formatted (goimports). Run 'goimports -w' or 'gofmt -w' to fix formatting.

🤖 Prompt for AI Agents 
In pkg/connector/connector.go around lines 124 to 126, the file is not formatted
with gofmt/goimports which breaks CI; run goimports -w
pkg/connector/connector.go (or gofmt -w pkg/connector/connector.go), ensure
imports are organized and the file is saved, then re-commit and push the change
so the pipeline can pass.

servicenowClient, err := servicenow.NewClient(httpClient, auth, deployment, ticketSchemaFilters)
if err != nil {
Expand Down
67 changes: 55 additions & 12 deletions pkg/servicenow/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"net/http"
"net/url"
"strings"
"time"

"github.com/tomnomnom/linkheader"
"google.golang.org/grpc/codes"
Expand Down Expand Up @@ -592,11 +593,25 @@
}
req = append(req, paginationVarsToReqOptions(&pg)...)

next, err := c.get(ctx, fmt.Sprintf(VariableSetM2MBaseUrl, c.deployment), &resp, req...)
if err != nil {
return nil, "", err
maxRetries := 3
baseDelay := 2 * time.Second

Check failure on line 598 in pkg/servicenow/client.go

View workflow job for this annotation

GitHub Actions / go-lint 

File is not properly formatted (goimports)
for attempt := 0; attempt < maxRetries; attempt++ {
next, err := c.get(ctx, fmt.Sprintf(VariableSetM2MBaseUrl, c.deployment), &resp, req...)
if err != nil {
if status.Code(err) == codes.DeadlineExceeded || strings.Contains(err.Error(), "timeout") || strings.Contains(err.Error(), "deadline exceeded") {
if attempt < maxRetries-1 {
delay := baseDelay * time.Duration(1<<attempt)
time.Sleep(delay)
continue
}
}
return nil, "", err
Comment on lines +599 to +609
Copy link

@coderabbitai coderabbitai bot Sep 26, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Retry condition misses HTTP gateway/request timeouts.

doRequest wraps HTTP failures with status.Error(codes.Code(status), "Request failed"). For 504/408 responses, status.Code(err) becomes codes.Code(504)/codes.Code(408) and the message lacks "timeout" wording, so these new retries never trigger and we still fail immediately on the common ServiceNow timeout scenarios. Please treat those HTTP statuses as retryable (and apply the fix to all three functions).

Apply this diff (pattern repeated in the other two retry blocks):

-		if err != nil {
-			if status.Code(err) == codes.DeadlineExceeded || strings.Contains(err.Error(), "timeout") || strings.Contains(err.Error(), "deadline exceeded") {
+		if err != nil {
+			code := status.Code(err)
+			if code == codes.DeadlineExceeded ||
+				code == codes.Code(http.StatusGatewayTimeout) ||
+				code == codes.Code(http.StatusRequestTimeout) ||
+				strings.Contains(err.Error(), "timeout") ||
+				strings.Contains(err.Error(), "deadline exceeded") {

Also applies to: 634-644, 667-677

🤖 Prompt for AI Agents 
In pkg/servicenow/client.go around lines 599-609 (and similarly apply to 634-644
and 667-677): the retry condition only checks for gRPC DeadlineExceeded or text
containing "timeout"/"deadline exceeded", but doRequest wraps HTTP 504/408
responses into status.Errors with numeric codes (e.g. codes.Code(504),
codes.Code(408)) and no "timeout" text, so those timeouts never trigger a retry.
Update the conditional to also treat HTTP 504 and 408 as retryable by checking
status.Code(err) against codes.Code(504) and codes.Code(408) (in addition to
codes.DeadlineExceeded and the existing string checks), then keep the existing
exponential backoff and continue logic; apply the same change to the two other
retry blocks at the specified lines.

}
return resp.Result, next, nil
}
return resp.Result, next, nil

return nil, "", fmt.Errorf("failed to get variable set links after %d retries", maxRetries)
}

func (c *Client) GetVariablesBySetIDs(ctx context.Context, setIDs []string, pg PaginationVars) ([]ItemOptionNew, string, error) {
Expand All @@ -611,11 +626,25 @@
}
req = append(req, paginationVarsToReqOptions(&pg)...)

next, err := c.get(ctx, fmt.Sprintf(ItemOptionNewBaseUrl, c.deployment), &resp, req...)
if err != nil {
return nil, "", err
maxRetries := 3
baseDelay := 2 * time.Second

for attempt := 0; attempt < maxRetries; attempt++ {
next, err := c.get(ctx, fmt.Sprintf(ItemOptionNewBaseUrl, c.deployment), &resp, req...)
if err != nil {
if status.Code(err) == codes.DeadlineExceeded || strings.Contains(err.Error(), "timeout") || strings.Contains(err.Error(), "deadline exceeded") {
if attempt < maxRetries-1 {
delay := baseDelay * time.Duration(1<<attempt)
time.Sleep(delay)
continue
}
}
return nil, "", err
}
return resp.Result, next, nil
}
return resp.Result, next, nil

return nil, "", fmt.Errorf("failed to get variables by set IDs after %d retries", maxRetries)
}

func (c *Client) GetChoicesForVariables(ctx context.Context, varIDs []string, pg PaginationVars) ([]QuestionChoice, string, error) {
Expand All @@ -630,11 +659,25 @@
}
req = append(req, paginationVarsToReqOptions(&pg)...)

next, err := c.get(ctx, fmt.Sprintf(QuestionChoiceBaseUrl, c.deployment), &resp, req...)
if err != nil {
return nil, "", err
maxRetries := 3
baseDelay := 2 * time.Second

for attempt := 0; attempt < maxRetries; attempt++ {
next, err := c.get(ctx, fmt.Sprintf(QuestionChoiceBaseUrl, c.deployment), &resp, req...)
if err != nil {
if status.Code(err) == codes.DeadlineExceeded || strings.Contains(err.Error(), "timeout") || strings.Contains(err.Error(), "deadline exceeded") {
if attempt < maxRetries-1 {
delay := baseDelay * time.Duration(1<<attempt)
time.Sleep(delay)
continue
}
}
return nil, "", err
}
return resp.Result, next, nil
}
return resp.Result, next, nil

return nil, "", fmt.Errorf("failed to get choices for variables after %d retries", maxRetries)
}

// Unused but consider switching to this to get both direct catalog item variables and variables from variable sets.
Expand Down
Loading